Upgrade plugin for OpenShift 4.19+ compatibility #113
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: empovit The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
WalkthroughConsolidates linting/style configs, upgrades Node/tooling and webpack, adds an i18n extraction pipeline (scripts, lexer, parser config), introduces a shared OverviewDetailItem component and styles, tightens TypeScript, adjusts UI/CSS and Docker files, and updates localization and dependency metadata. Changes
Sequence Diagram(s)sequenceDiagram
actor Dev/CI
participant Shell as ./i18n-scripts/build-i18n.sh
participant Parser as i18next-parser
participant Lexer as CustomJSONLexer
participant FS as filesystem (src + locales)
participant Node as set-english-defaults.js
Dev/CI->>Shell: run build-i18n.sh
Shell->>Parser: invoke i18next-parser with pattern & config
Parser->>Lexer: request extraction for JSON/comment-JSON files
Lexer->>Lexer: parse comment-JSON & collect %KEY% tokens
Lexer-->>Parser: return extracted keys
Parser->>FS: write locales/$LOCALE/$NAMESPACE.json
Dev/CI->>Node: run set-english-defaults.js
Node->>FS: read locales/en/*, update empty values, write back
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
empovit
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/GPUDashboard/GPUSelector.tsx (1)
24-31: Prevent selection of loading and error states.The loading and error state options can still be selected by users, which would set invalid values (like "Loading GPUs" or "No GPU found") as the selected GPU UUID. These options should be explicitly disabled to prevent selection.
🐛 Proposed fix to disable loading/error options
if (!gpusLoaded) { items = [ - <SelectOption key="disabled" value={t('Loading GPUs')}> + <SelectOption key="disabled" value={t('Loading GPUs')} isDisabled> <Spinner size="md" /> {t('Loading GPUs')} </SelectOption>, ]; } else if (!gpus?.length || gpusError) { - items = [<SelectOption key="disabled" value={t('No GPU found')} />]; + items = [<SelectOption key="disabled" value={t('No GPU found')} isDisabled />]; } else {
🧹 Nitpick comments (10)
src/utils/units.ts (1)
209-217: Cache the numeric conversion to avoid repeatedNumber(v)calls.The function calls
Number(v)three times. For consistency withhumanizeRatioandhumanizePercentage(which usenumValue), cache the conversion result.♻️ Suggested refactor
export const humanizeCpuCores: HumanizeFunc = (v) => { - const value = Number(v) < 1 ? round(Number(v) * 1000) : Number(v); - const unit = Number(v) < 1 ? 'm' : ''; + const numValue = Number(v); + const value = numValue < 1 ? round(numValue * 1000) : numValue; + const unit = numValue < 1 ? 'm' : ''; return { string: `${formatValue(value)}${unit}`, unit, value, }; };src/components/PodStatus/PodStatus.tsx (1)
116-133: Consider consistent paragraph styling.The informational text uses
<Content component="p">(lines 117-124) while the links section uses plain<p>(lines 125-133). For consistency, consider applying the same approach throughout, unless the distinction is intentional to differentiate content types.♻️ Proposed refactor for consistency
- <p> + <Content component="p"> <Link to={`/k8s/ns/${pod.metadata.namespace}/pods/${pod.metadata.name}/logs`}> {t('View logs')} </Link>   <Link to={`/k8s/ns/${pod.metadata.namespace}/pods/${pod.metadata.name}/events`}> {t('View events')} </Link> - </p> + </Content>src/components/shared/OverviewDetailItem.tsx (1)
33-33: Consider avoiding trailing space in className.The current template literal will produce a trailing space when
valueClassNameis undefined or empty. While harmless, it's slightly cleaner to avoid it.♻️ Optional refactor to remove trailing space
- <dd className={`ng-overview-detail-item__value ${valueClassName || ''}`}>{value}</dd> + <dd className={valueClassName ? `ng-overview-detail-item__value ${valueClassName}` : 'ng-overview-detail-item__value'}>{value}</dd>Alternative using array approach:
- <dd className={`ng-overview-detail-item__value ${valueClassName || ''}`}>{value}</dd> + <dd className={['ng-overview-detail-item__value', valueClassName].filter(Boolean).join(' ')}>{value}</dd>src/components/GPUDashboard/GPUSelector.tsx (1)
33-35: Consider displaying GPU name instead of UUID.Line 54 displays
selectedGPU?.uuidin the MenuToggle, which shows a UUID to users. Consider displaying a more user-friendly GPU name or model if available in the GPU object.♻️ Optional enhancement for better UX
If the
gpuobject has anameormodelNameproperty, consider:} else { items = gpus.map((gpu) => ( - <SelectOption key={gpu.uuid} value={gpu.uuid} description={`Model: ${gpu.modelName}`} /> + <SelectOption key={gpu.uuid} value={gpu.uuid}> + {gpu.modelName || gpu.uuid} + </SelectOption> )); }And update the toggle display:
> - {selectedGPU?.uuid || t('Select GPU')} + {selectedGPU?.modelName || selectedGPU?.uuid || t('Select GPU')} </MenuToggle>i18n-scripts/lexers.js (1)
1-30: CustomJSONLexer implementation looks correct.The lexer correctly:
- Uses
comment-jsonto parse JSON with comments support- Employs a reviver function to extract localization keys matching the
%KEY%pattern- Handles errors gracefully by logging and returning an empty array
- Returns keys in the expected
{ key: ... }format for i18next-parserThe class extends
EventEmitterbut doesn't emit events. This is likely for interface compatibility with i18next-parser lexer expectations, though it could be removed if not required by the framework.♻️ Optional: Remove EventEmitter if not required by framework
If i18next-parser doesn't require lexers to extend EventEmitter, you can simplify:
-const EventEmitter = require('events'); const jsonc = require('comment-json'); /** * Custom JSON parser for localizing keys matching format: /%.+%/ */ -module.exports.CustomJSONLexer = class extends EventEmitter { +module.exports.CustomJSONLexer = class { extract(content, filename) {i18next-parser.config.js (1)
1-4: Consider configuring ESLint environment instead of inline disables.The multiple
eslint-disablecomments work but suggest ESLint isn't configured for Node.js CommonJS files. Consider adding a.eslintrcoverride for.jsfiles or settingenv: { node: true }to properly recognizerequireandmodule.♻️ Alternative: Configure ESLint for Node.js environment
In your ESLint configuration, add an override for
.jsfiles:{ "overrides": [ { "files": ["*.js"], "env": { "node": true }, "rules": { "@typescript-eslint/no-var-requires": "off" } } ] }This would eliminate the need for inline disable comments.
webpack.config.ts (1)
8-8: Mixed import styles: consider standardizing.The file uses ES6 imports throughout except for CopyWebpackPlugin. If possible, standardize to ES6 imports for consistency.
♻️ Standardize to ES6 import
-const CopyWebpackPlugin = require('copy-webpack-plugin'); +import CopyWebpackPlugin from 'copy-webpack-plugin';Note: Only apply if compatible with your TypeScript configuration and plugin version.
i18n-scripts/common.js (2)
14-26: Consider propagating errors to callers.Errors are logged but the Promise doesn't reject, preventing callers from detecting failures. For build scripts, propagating errors allows proper failure handling.
♻️ Propagate errors to enable proper failure handling
parseFolder(directory, argFunction, packageDir) { return (async () => { try { const files = await fs.promises.readdir(directory); for (const file of files) { const filePath = path.join(directory, file); await argFunction(filePath, packageDir); } } catch (e) { console.error(`Failed to parseFolder ${directory}:`, e); + throw e; } })(); },This allows callers to handle failures appropriately, such as exiting with a non-zero status in build scripts.
27-33: Consider propagating errors for consistency.Like
parseFolder, errors are logged but not thrown. For consistency and to allow callers to handle failures, consider propagating errors.♻️ Optional: propagate errors
deleteFile(filePath) { try { fs.unlinkSync(filePath); } catch (e) { console.error(`Failed to delete file ${filePath}:`, e); + throw e; } },Note: Only apply if file deletion failures should halt the build process.
i18n-scripts/set-english-defaults.js (1)
74-76: Consider adding error exit for build failures.If locale processing fails, the script continues without exiting with a non-zero status. Build scripts should typically fail explicitly to prevent incomplete builds.
♻️ Exit with error status on failure
(async () => { - await common.parseFolder(publicDir, processLocalesFolder); + try { + await common.parseFolder(publicDir, processLocalesFolder); + } catch (error) { + console.error('Failed to set English defaults:', error); + process.exit(1); + } })();This requires updating
common.jsto propagate errors (see previous comment on that file).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (29)
.eslintrc.eslintrc.yml.gitignore.prettierrc.yml.stylelintrc.yamlDockerfileREADME.mddeployment/console-plugin-nvidia-gpu/Chart.yamli18n-scripts/build-i18n.shi18n-scripts/common.jsi18n-scripts/lexers.jsi18n-scripts/set-english-defaults.jsi18next-parser.config.jspackage.jsonsrc/components/ClusterOverview/GPUProviders.tsxsrc/components/ClusterOverview/GPUStatus.tsxsrc/components/GPUDashboard/Cards/InfoCard.tsxsrc/components/GPUDashboard/Cards/MetricsCard.tsxsrc/components/GPUDashboard/Cards/WorkloadsCard.csssrc/components/GPUDashboard/Cards/WorkloadsCard.tsxsrc/components/GPUDashboard/GPUDashboard.tsxsrc/components/GPUDashboard/GPUSelector.tsxsrc/components/NotAvailable.tsxsrc/components/PodStatus/PodStatus.tsxsrc/components/shared/OverviewDetailItem.csssrc/components/shared/OverviewDetailItem.tsxsrc/utils/units.tstsconfig.jsonwebpack.config.ts
💤 Files with no reviewable changes (1)
- .eslintrc
🧰 Additional context used
🧬 Code graph analysis (3)
i18n-scripts/lexers.js (2)
i18next-parser.config.js (1)
require(2-2)i18n-scripts/set-english-defaults.js (1)
keys(34-34)
src/components/GPUDashboard/GPUDashboard.tsx (1)
src/components/GPUDashboard/Cards/GPUDashboardGraphs.tsx (1)
GPUDashboardGraphs(48-146)
i18n-scripts/set-english-defaults.js (3)
i18n-scripts/common.js (2)
fs(1-1)path(2-2)i18next-parser.config.js (1)
require(2-2)i18n-scripts/lexers.js (1)
keys(9-9)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: lint-test
🔇 Additional comments (46)
src/components/NotAvailable.tsx (1)
8-8: LGTM! Correct PatternFly v6 class migration.The CSS class update from
pf-u-color-200topf-v6-u-color-200correctly follows PatternFly v6 naming conventions. The component functionality remains unchanged.src/components/GPUDashboard/Cards/MetricsCard.tsx (1)
95-95: LGTM! PatternFly v6 utility class update is correct.The class name update from
pf-u-text-align-centertopf-v6-u-text-align-centeris consistent with the PatternFly v4 → v6 upgrade described in the PR objectives.src/utils/units.ts (4)
179-187: LGTM!The type broadening to accept
string | numberaligns with the SDK'sHumanizetype and accommodates string values commonly returned from Prometheus queries.
189-208: LGTM!Consistent coercion pattern applied across all helpers. The internal
humanizefunction already handles non-finite values, so invalid string inputs gracefully fall back to0.
219-232: LGTM!Clean implementation with proper handling of non-finite values and -0. The
numValuecaching pattern is consistent and efficient.
234-245: LGTM!Consistent implementation pattern with
humanizeRatio. Proper handling of edge cases.src/components/PodStatus/PodStatus.tsx (2)
3-3: LGTM! PatternFly v6 migration applied correctly.The addition of
Contentcomponent and removal of deprecated Text components aligns with the PatternFly v6 upgrade mentioned in the PR objectives.
126-126: Excellent catch! Critical typo fixed.The correction from
pod.metadata.namtopod.metadata.namefixes broken URLs that would have prevented navigation to pod logs, events, and debug container pages.Also applies to: 130-130, 140-140
src/components/GPUDashboard/Cards/InfoCard.tsx (1)
3-3: LGTM! Import path updated correctly.The import has been successfully migrated from the external package to the local shared module. The usage throughout the file is consistent with the new component's API.
src/components/ClusterOverview/GPUProviders.tsx (1)
2-2: LGTM! Import path correctly updated.The migration from the external package to the local shared module is consistent and correct.
src/components/GPUDashboard/GPUDashboard.tsx (1)
26-55: LGTM! Layout simplified appropriately.The removal of the Page wrapper and nested PageSection simplifies the component structure while preserving all functionality. The internal layout hierarchy and component composition remain intact.
src/components/shared/OverviewDetailItem.css (1)
1-18: LGTM! CSS follows PatternFly v6 conventions.The stylesheet correctly:
- Uses PatternFly v6 design tokens for consistency
- Follows the ng- prefix naming convention as specified in PR objectives
- Implements proper text wrapping (lines 16-17) to handle long content gracefully
- Provides appropriate semantic styling for the dl/dt/dd structure
src/components/shared/OverviewDetailItem.tsx (1)
1-36: LGTM! Well-structured component with good semantic HTML.The component implementation is solid:
- Props interface is clean and matches all usage patterns
- Semantic dl/dt/dd HTML is appropriate for key-value pairs
- Loading/error states are properly handled with PatternFly v6 components
- CSS module integration follows best practices
src/components/GPUDashboard/Cards/WorkloadsCard.css (1)
4-9: LGTM! CSS naming convention correctly applied.The changes properly implement the ng- prefix naming convention mentioned in the PR objectives. The
.ng-break-wordclass withword-breakandoverflow-wrapproperties is appropriate for handling long text content in table cells.src/components/GPUDashboard/Cards/WorkloadsCard.tsx (2)
49-54: LGTM! CSS class names consistently updated.All table column class names have been consistently updated from
co-break-wordtong-break-word, correctly following the ng- prefix naming convention and matching the CSS file changes.
42-42: PatternFly v6 EmptyState icon prop usage is correct.The
icon={CubesIcon}prop is the correct PatternFly v6 API. TheEmptyStateIconcomponent is no longer directly imported—it's now internal toEmptyStateand automatically wraps the icon component you provide.src/components/ClusterOverview/GPUStatus.tsx (2)
115-115: LGTM! Icon color migration looks correct.The updated usage of
okColordirectly (removing.value) aligns with the PatternFly v4→v6 migration. PatternFly v6 icons should properly handle CSS variable strings in thecolorprop.Also applies to: 150-150
18-19: The CSS variable and fallback color are correct. The PatternFly v6 token--pf-t--global--color--status--success--defaultand the fallback color#3D7317match PatternFly v6 documentation. No centralized token constants file exists in the codebase, so the hardcoded approach is consistent with project conventions.src/components/GPUDashboard/GPUSelector.tsx (4)
22-22: Good type refinement for strict mode.The more specific
React.ReactElement<SelectOptionProps>[]typing aligns well with TypeScript strict mode and provides better type safety.
47-56: MenuToggle implementation looks correct.The custom toggle implementation with
MenuToggleproperly manages refs, click handlers, and accessibility attributes (isExpanded,isDisabled).
2-10: The PatternFly v6 Select imports and implementation are correct. All imported components (SelectOption, SelectList, SelectOptionProps, Select, MenuToggle, MenuToggleElement) are valid exports from @patternfly/react-core and are used properly in the composable Select pattern with correct type safety and handlers.
42-45: The type assertion is safe in this context.The
as stringassertion on line 43 is not actually unsafe. AllSelectOptionvalues used in this component are strings (both the translated strings on lines 26 and 31, and the GPU UUIDs on line 34). More importantly, the Select component is disabled (viaisDisabledon line 20) when the loading and error options are shown, so theonSelecthandler is never invoked with those translated strings—only with actual GPU UUID strings. PatternFly v6'sonSelectsignature acceptsstring | number, and in this case, only strings are used. The type assertion is redundant but not a safety concern.Likely an incorrect or invalid review comment.
.gitignore (1)
77-77: Pattern addition looks good.The
.cache_*addition extends parcel-bundler cache ignores to cover dot-variant naming patterns, which aligns with the updated build environment..prettierrc.yml (1)
5-6: Verify Prettier config simplification doesn't change formatting behavior.The removed options (tabWidth, semi, jsxSingleQuote, proseWrap) may have defaults in Prettier that differ from the previous explicit values. Confirm that relying on Prettier's defaults for these options won't unexpectedly change formatting behavior across the codebase.
.eslintrc.yml (1)
1-26: ESLint config is well-structured for TypeScript + React.The configuration appropriately layers ESLint recommendations, React and TypeScript plugins, and Prettier integration. Disabling
react/prop-typesin favor of TypeScript's type checking and using auto-detect for React version are modern best practices.One note:
ecmaVersion: latest(line 13) will automatically pick up new ECMAScript versions as ESLint/parser support evolves. For strict reproducibility, consider pinning to a specific version (e.g., 2024). However, given this PR's modernization scope, usinglatestis likely intentional.i18n-scripts/build-i18n.sh (1)
1-7: LGTM! Script follows shell best practices.The script correctly uses strict error handling (
set -exuo pipefail) and properly escapes the i18next template variables (\$LOCALE,\$NAMESPACE) to prevent shell expansion. The file pattern and command invocation are appropriate for the i18n extraction workflow.Dockerfile (2)
1-1: Upgrade to UBI9 and Node.js 20 aligns with PR objectives.The base image upgrade from
ubi8/nodejs-16toubi9/nodejs-20correctly implements the Node.js 20 upgrade mentioned in the PR objectives.
5-11: Build paths correctly updated for new working directory.The change from root directory to
/usr/src/appas the working directory is a good practice, and the artifact copy path in line 11 (--from=build /usr/src/app/dist) correctly reflects this new structure.tsconfig.json (2)
7-12: TypeScript strict mode enabled as per PR objectives.The addition of
strict: true(line 10) correctly implements the PR objective to enable TypeScript strict mode. ThesourceMap: true(line 7) aids debugging, andskipLibCheck: true(line 12) is a common practice to improve compilation performance.
16-22: ts-node configuration supports new i18n tooling.The ts-node block enables in-project TypeScript execution with CommonJS module resolution, which aligns with the new i18n-scripts infrastructure introduced in this PR. The
transpileOnly: truesetting is appropriate for build scripts where faster compilation is prioritized.i18next-parser.config.js (2)
6-6: Good addition: Catalog sorting improves consistency.Enabling
sort: trueensures translation keys are consistently ordered in the generated catalog files, making diffs cleaner and improving maintainability.
15-30: Mixed lexer reference pattern is correct and follows i18next-parser convention.The lexers configuration correctly uses string references for built-in lexers (
'HandlebarsLexer','HTMLLexer','JavascriptLexer') and a direct class reference for the custom lexer (CustomJSONLexer). This is the documented and intended pattern for i18next-parser: pass string names for built-in lexers and pass the class/function directly when using custom lexers..stylelintrc.yaml (1)
1-27: LGTM! Well-structured Stylelint configuration for PatternFly v6.The configuration appropriately enforces dark mode compatibility and plugin CSS isolation. The rules correctly prevent:
- Direct color values (enforcing PF CSS variables)
- Reserved prefixes (.pf-, .co-)
- Naked element selectors that could impact console layout
The comments clearly explain the rationale for each rule.
webpack.config.ts (5)
43-44: LGTM! SCSS loader chain correctly configured.The loader chain (sass-loader → css-loader → style-loader) follows webpack best practices for SCSS processing.
53-58: Good addition for module compatibility.Setting
fullySpecified: falseimproves compatibility with modules that don't use full file extensions in imports, which is a common webpack 5 migration pattern.
64-69: LGTM! DevServer configuration appropriate for development.The permissive
allowedHosts: 'all'and CORS headers are acceptable since they only apply to the development server, not production builds.
76-78: LGTM! CopyWebpackPlugin correctly configured for locale files.The plugin will copy locale files from the source
localesdirectory todist/locales, which aligns with the i18n infrastructure added in this PR.
80-84: LGTM! Production optimization settings properly configured.The configuration appropriately toggles between development (source maps, named chunks) and production (no source maps, deterministic chunks, minification) modes.
package.json (4)
5-8: LGTM! Node.js 20 requirement aligns with modern tooling.The Node.js 20 engine requirement is appropriate for the updated dependencies and aligns with the Dockerfile upgrade mentioned in the PR objectives.
9-20: LGTM! Script updates align with modernized tooling.The updated scripts properly support:
- Production builds with explicit NODE_ENV
- TypeScript configuration execution via ts-node
- Integrated i18n workflow
- CSS linting with stylelint
84-86: LGTM! Correct peerDependency declaration for plugin architecture.Declaring i18next as a peerDependency is appropriate for a console plugin, ensuring the host console provides a single i18next instance.
22-63: No action needed. PatternFly v6 (including 6.2.2) officially supports React 17 and follows a "two most recent React versions" policy. OpenShift Console SDK 1.4.0 is designed for React 17.x, making this the intended combination. The dependency versions are compatible.i18n-scripts/common.js (1)
5-13: LGTM! Synchronous operation acceptable for build scripts.Using
lstatSyncis appropriate for build-time scripts where blocking is acceptable. The error handling correctly returnsfalsefor non-existent paths.i18n-scripts/set-english-defaults.js (3)
8-19: LGTM! Rule determination follows i18next conventions.The priority order (WithCount_plural → WithCount → _plural → default) correctly matches i18next naming patterns. The use of
includes()is appropriate given standard i18next key conventions.
21-66: LGTM! Transformation logic correctly implements i18next conventions.The function properly:
- Validates file structure
- Preserves existing translations (only fills empty strings)
- Applies appropriate transformations based on key naming patterns
- Uses the pluralize library for English pluralization
The file overwriting behavior is acceptable for a build script.
68-72: LGTM! Correctly targets only English locale for defaults.Filtering by
basename === 'en'ensures the script only modifies English locale files, leaving other locales for human translators.
empovit
force-pushed
the
update-for-ocp-4.19-no-chart-changes
branch
from
bd88e0f to
6107e33
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/GPUDashboard/GPUSelector.tsx (1)
25-31: AddisDisabledto placeholder SelectOptions to prevent selection.The loading and "No GPU found" options are selectable, which could cause unexpected behavior if a user clicks them. These placeholder items should be disabled.
Proposed fix
if (!gpusLoaded) { items = [ - <SelectOption key="disabled" value={t('Loading GPUs')}> + <SelectOption key="disabled" value={t('Loading GPUs')} isDisabled> <Spinner size="md" /> {t('Loading GPUs')} </SelectOption>, ]; } else if (!gpus?.length || gpusError) { - items = [<SelectOption key="disabled" value={t('No GPU found')} />]; + items = [<SelectOption key="disabled" value={t('No GPU found')} isDisabled />]; } else {
🤖 Fix all issues with AI agents
In @deployment/console-plugin-nvidia-gpu/Chart.yaml:
- Line 4: The Chart.yaml metadata still lists version: 0.2.7 while the PR
updates the OpenShift requirement to 4.19+; update the Chart.yaml "version"
field to 0.3.0 to reflect this breaking change (and search the chart for any
other occurrences of the old chart version to update them as well); do not
change the descriptive text you already updated, just bump the Chart.yaml
version entry (and ensure any release notes or changelog entries, if present,
reflect the version bump).
In @package.json:
- Around line 24-27: The dependency @patternfly/react-charts is pinned to v7
which targets PatternFly 5; update the package version string
"@patternfly/react-charts" to use the v8 semver range (e.g., ^8.x) so it aligns
with the PatternFly 6 packages (@patternfly/react-core, @patternfly/react-icons,
@patternfly/react-table); update package.json accordingly and run
install/lockfile update to ensure dependency resolution and tests pass.
- Line 61: The package.json currently pins webpack to the vulnerable version
"5.75.0"; update the dependency entry for "webpack" to a fixed release (at
minimum ">=5.76.0", preferably "5.94.0" or the latest 5.x) and then regenerate
your lockfile (npm install / yarn install) so the new version is installed;
after updating, run the test suite and a security audit (npm audit / yarn audit)
to verify the vulnerability is resolved.
🧹 Nitpick comments (6)
src/components/GPUDashboard/GPUSelector.tsx (1)
33-35: Consider displaying GPU model name instead of UUID for better UX.The
SelectOptionhas no children, so PF v6 displaysvalue(the UUID) as the option label. Combined with theMenuTogglealso showingselectedGPU?.uuid, users see raw UUIDs rather than human-readable GPU names.Proposed improvement
} else { items = gpus.map((gpu) => ( - <SelectOption key={gpu.uuid} value={gpu.uuid} description={`Model: ${gpu.modelName}`} /> + <SelectOption key={gpu.uuid} value={gpu.uuid} description={gpu.uuid}> + {gpu.modelName} + </SelectOption> )); }And update the toggle display:
<MenuToggle ref={toggleRef} onClick={() => setOpen(!isOpen)} isExpanded={isOpen} isDisabled={isDisabled} > - {selectedGPU?.uuid || t('Select GPU')} + {selectedGPU?.modelName || t('Select GPU')} </MenuToggle>Also applies to: 54-54
.prettierrc.yml (1)
1-6: LGTM - Config simplification looks good.The removal of redundant keys (
tabWidth,semi,jsxSingleQuote,proseWrap) in favor of Prettier's sensible defaults is a reasonable cleanup.Minor nit: there are two trailing blank lines at the end of the file which could be reduced to one.
i18n-scripts/common.js (1)
14-25: UnusedpackageDirparameter and silent error handling.
The
packageDirparameter is passed toargFunctionbut looking at the usage inset-english-defaults.js,updateFileonly acceptsfileName. This parameter appears unused.Errors in
parseFolderare logged but swallowed, which may hide failures during the i18n build process.♻️ Suggested improvement
- parseFolder(directory, argFunction, packageDir) { + parseFolder(directory, argFunction) { return (async () => { try { const files = await fs.promises.readdir(directory); for (const file of files) { const filePath = path.join(directory, file); - await argFunction(filePath, packageDir); + await argFunction(filePath); } } catch (e) { console.error(`Failed to parseFolder ${directory}:`, e); + throw e; // Propagate error to caller } })(); },i18n-scripts/set-english-defaults.js (2)
8-19: UseendsWith()for more precise suffix matching.Using
includes()could match keys likemyWithCount_pluralKeyincorrectly. Since these are suffixes,endsWith()would be more accurate.♻️ Suggested improvement
function determineRule(key) { - if (key.includes('WithCount_plural')) { + if (key.endsWith('WithCount_plural')) { return 0; } - if (key.includes('WithCount')) { + if (key.endsWith('WithCount')) { return 1; } - if (key.includes('_plural')) { + if (key.endsWith('_plural')) { return 2; } return 3; }
21-27: Node.jsrequire()caches modules, which may cause stale data.If
updateFileis called multiple times on the same file (e.g., during iterative runs),require()will return the cached version from the first load, not the updated file content. Consider usingfs.readFileSyncwithJSON.parseinstead.♻️ Suggested fix
function updateFile(fileName) { let file; try { - file = require(fileName); + const content = fs.readFileSync(fileName, 'utf-8'); + file = JSON.parse(content); } catch (err) { throw new Error(`Failed to load ${fileName}: ${err.message}`); }src/utils/units.ts (1)
209-217: Avoid repeatedNumber(v)coercion calls.
Number(v)is called four times on the same input. Store the coerced value once for clarity and minor efficiency gains.♻️ Suggested refactor
export const humanizeCpuCores: HumanizeFunc = (v) => { - const value = Number(v) < 1 ? round(Number(v) * 1000) : Number(v); - const unit = Number(v) < 1 ? 'm' : ''; + const num = Number(v); + const value = num < 1 ? round(num * 1000) : num; + const unit = num < 1 ? 'm' : ''; return { string: `${formatValue(value)}${unit}`, unit, value, }; };
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (29)
.eslintrc.eslintrc.yml.gitignore.prettierrc.yml.stylelintrc.yamlDockerfileREADME.mddeployment/console-plugin-nvidia-gpu/Chart.yamli18n-scripts/build-i18n.shi18n-scripts/common.jsi18n-scripts/lexers.jsi18n-scripts/set-english-defaults.jsi18next-parser.config.jspackage.jsonsrc/components/ClusterOverview/GPUProviders.tsxsrc/components/ClusterOverview/GPUStatus.tsxsrc/components/GPUDashboard/Cards/InfoCard.tsxsrc/components/GPUDashboard/Cards/MetricsCard.tsxsrc/components/GPUDashboard/Cards/WorkloadsCard.csssrc/components/GPUDashboard/Cards/WorkloadsCard.tsxsrc/components/GPUDashboard/GPUDashboard.tsxsrc/components/GPUDashboard/GPUSelector.tsxsrc/components/NotAvailable.tsxsrc/components/PodStatus/PodStatus.tsxsrc/components/shared/OverviewDetailItem.csssrc/components/shared/OverviewDetailItem.tsxsrc/utils/units.tstsconfig.jsonwebpack.config.ts
💤 Files with no reviewable changes (1)
- .eslintrc
🚧 Files skipped from review as they are similar to previous changes (14)
- src/components/ClusterOverview/GPUStatus.tsx
- .stylelintrc.yaml
- src/components/GPUDashboard/GPUDashboard.tsx
- src/components/shared/OverviewDetailItem.tsx
- src/components/ClusterOverview/GPUProviders.tsx
- i18n-scripts/lexers.js
- tsconfig.json
- src/components/GPUDashboard/Cards/WorkloadsCard.css
- src/components/GPUDashboard/Cards/WorkloadsCard.tsx
- .gitignore
- src/components/shared/OverviewDetailItem.css
- src/components/PodStatus/PodStatus.tsx
- src/components/GPUDashboard/Cards/InfoCard.tsx
- README.md
🧰 Additional context used
🧬 Code graph analysis (1)
i18n-scripts/set-english-defaults.js (2)
i18n-scripts/common.js (1)
path(2-2)i18n-scripts/lexers.js (1)
keys(9-9)
🔇 Additional comments (26)
src/components/GPUDashboard/GPUSelector.tsx (2)
2-10: LGTM! Imports are correct for PatternFly v6 Select pattern.The imports align with the PF v6 controlled Select pattern using
MenuToggleas the toggle renderer.
38-59: LGTM! Select implementation follows PF v6 patterns correctly.The controlled Select with
MenuToggletoggle renderer,onOpenChange, andonSelecthandlers is implemented correctly per PatternFly v6 conventions.src/components/GPUDashboard/Cards/MetricsCard.tsx (1)
95-95: PatternFly v6 utility class naming is correct.The migration from v4 to v6 utility classes correctly follows the
pf-v6-u-*naming convention for the text alignment utility..eslintrc.yml (1)
1-27: LGTM - Well-structured ESLint configuration.The configuration follows best practices:
- Correct extends order with
prettierlast to disable conflicting formatting rules- TypeScript parser with JSX support properly configured
react/prop-types: offis appropriate since TypeScript handles type checking- React version auto-detection avoids manual maintenance
Minor nit: Two trailing blank lines at the end.
i18next-parser.config.js (1)
1-31: LGTM - i18next-parser configuration properly extended.The configuration adds appropriate lexer mappings for the project's file types. The
sort: trueoption ensures deterministic output ordering, which helps with diff readability in version control. TheCustomJSONLexerimport from./i18n-scripts/lexersis correctly defined and exported as a class extending EventEmitter.Dockerfile (1)
1-13: LGTM - Dockerfile properly upgraded for Node.js 20.The changes align well with the PR objectives:
- UBI9 base images (ubi9/nodejs-20 and ubi9/nginx-120) exist and are current
- Path changes to
/usr/src/appfollow standard conventions- Build artifacts are correctly copied to the nginx runtime stage
webpack.config.ts (7)
8-10: LGTM on production environment detection.The
isProdflag based onNODE_ENVis a standard pattern for webpack configurations. This enables conditional bundling behavior throughout the config.
22-23: Good use of content hashing for cache busting.Production builds use
[hash]and[chunkhash]for long-term caching, while development uses readable names for easier debugging.
43-44: SCSS support added correctly.The loader chain
style-loader → css-loader → sass-loaderis the correct order for processing SCSS files.
53-58: ESM interop rule is appropriate.Setting
fullySpecified: falsefor.mjsand.jsfiles resolves ESM module resolution issues that can occur with some dependencies.
64-64: Security consideration:allowedHosts: 'all'in devServer.This setting disables host header checking, which is acceptable for local development but ensure this configuration is never used in production-exposed environments. The current setup is development-only, so this is fine.
80-84: LGTM on production optimization settings.
devtool: falsein production avoids shipping source mapschunkIds: 'deterministic'ensures stable chunk IDs for cachingminimize: isProdenables minification only in production
76-78: Thepatternsarray syntax is correct and fully supported bycopy-webpack-plugin@^6.4.1. The code follows the official syntax for this version, and thelocalesdirectory exists in the repository. No action needed.Likely an incorrect or invalid review comment.
i18n-scripts/common.js (1)
5-12: LGTM onisDirectoryhelper.The try-catch correctly handles the case where
lstatSyncthrows for non-existent paths, returningfalseas a safe fallback.i18n-scripts/set-english-defaults.js (3)
29-31: LGTM on input validation.Good defensive check to ensure the loaded file is a valid object and not an array or primitive.
44-59: LGTM on i18next plural rule transformations.The transformation logic correctly follows i18next conventions for plural keys (
_plural,WithCount,WithCount_plural). The use of thepluralizelibrary for generating plural forms is appropriate.
68-76: LGTM on locale processing flow.The script correctly filters for the
endirectory and processes only English locale files. The self-invoking async pattern at startup is appropriate for a CLI script.package.json (4)
5-8: LGTM on license and engine specification.Adding the Apache-2.0 license and requiring Node.js >= 20 aligns with the PR objectives for OpenShift 4.19+ compatibility.
84-86: Clarifyi18nextin both devDependencies and peerDependencies.
i18nextappears in bothdevDependencies(line 42) andpeerDependencies(line 85). This is valid if the plugin expects the host application to providei18nextat runtime while using it during development. However, ensure this is intentional.
42-43: No action needed—i18next-parser@^3.11.0 provides the i18next CLI.The package already includes the
i18nextbinary (bin/cli.js), so the build script dependency is correct.
48-49: No action needed—PatternFly v6 is compatible with React 17.PatternFly v6 supports both React 17 and React 18 per its documented version policy (supporting two most recent React versions). React ^17.0.1 is compatible with this upgrade.
Likely an incorrect or invalid review comment.
i18n-scripts/build-i18n.sh (1)
1-7: LGTM on i18n extraction script.Good use of strict shell options (
set -exuo pipefail) for fail-fast behavior. The file pattern correctly includes JSON files to work with theCustomJSONLexer. The script already has execute permissions and i18next-parser is available as a dependency in package.json.src/components/NotAvailable.tsx (1)
8-8: Update CSS class to valid PatternFly v6 utility.The class
.pf-v6-u-color-200does not exist in PatternFly v6. PatternFly v6 text color utilities use semantic naming (e.g.,.pf-v6-u-text-color-subtle,.pf-v6-u-text-color-regular,.pf-v6-u-text-color-disabled).For secondary/subtle text (the equivalent of v4's
.pf-u-color-200), use.pf-v6-u-text-color-subtle.Likely an incorrect or invalid review comment.
src/utils/units.ts (3)
179-187: LGTM!Expanding the input type to
string | numberis appropriate for handling values from APIs that may return numeric data as strings.
189-208: LGTM!The consistent use of
Number(v)for coercion is correct. Invalid inputs likeNaNare properly handled by theisFinitechecks in the internalhumanizeandformatValuefunctions, defaulting to0.
219-245: LGTM!The refactored
humanizeRatioandhumanizePercentagefunctions correctly normalize inputs and handle edge cases (-0, non-finite values). The logic properly distinguishes between ratio inputs (0-1 range) and percentage inputs (0-100 range). The code compiles without errors in strict TypeScript mode, confirming that the SDK'sHumanizetype is compatible with these function signatures.
empovit
force-pushed
the
update-for-ocp-4.19-no-chart-changes
branch
from
6107e33 to
3fc937c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/utils/units.ts:
- Around line 209-218: The humanizeCpuCores function must normalize invalid
numeric input like other helpers: detect when Number(v) is NaN and treat it as 0
before computing value/unit/string. Update humanizeCpuCores to set numValue =
Number(v) and if Number.isNaN(numValue) then set numValue = 0 (or use a guarded
normalized variable), then compute value = numValue < 1 ? round(numValue * 1000)
: numValue, unit = numValue < 1 ? 'm' : '', and return the formatted string
using formatValue so that string and value remain consistent.
🧹 Nitpick comments (2)
i18n-scripts/set-english-defaults.js (2)
8-19: Consider using named constants instead of magic numbers for clarity.The return values 0, 1, 2, 3 lack semantic meaning. Using named constants would improve readability and maintainability.
♻️ Suggested improvement
+const RULE = { + WITH_COUNT_PLURAL: 0, + WITH_COUNT: 1, + PLURAL: 2, + DEFAULT: 3, +}; + function determineRule(key) { if (key.includes('WithCount_plural')) { - return 0; + return RULE.WITH_COUNT_PLURAL; } if (key.includes('WithCount')) { - return 1; + return RULE.WITH_COUNT; } if (key.includes('_plural')) { - return 2; + return RULE.PLURAL; } - return 3; + return RULE.DEFAULT; }Then update the switch cases to use
RULE.WITH_COUNT_PLURAL, etc.
21-27: Usingrequire()for JSON data files can cause caching issues.Node's
require()caches loaded modules, so if this file is processed again (e.g., in watch mode or rerun), you'll get stale data. Usefs.readFileSyncwithJSON.parseinstead for predictable behavior.♻️ Suggested fix
function updateFile(fileName) { let file; try { - file = require(fileName); + const content = fs.readFileSync(fileName, 'utf8'); + file = JSON.parse(content); } catch (err) { throw new Error(`Failed to load ${fileName}: ${err.message}`); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (29)
.eslintrc.eslintrc.yml.gitignore.prettierrc.yml.stylelintrc.yamlDockerfileREADME.mddeployment/console-plugin-nvidia-gpu/Chart.yamli18n-scripts/build-i18n.shi18n-scripts/common.jsi18n-scripts/lexers.jsi18n-scripts/set-english-defaults.jsi18next-parser.config.jspackage.jsonsrc/components/ClusterOverview/GPUProviders.tsxsrc/components/ClusterOverview/GPUStatus.tsxsrc/components/GPUDashboard/Cards/InfoCard.tsxsrc/components/GPUDashboard/Cards/MetricsCard.tsxsrc/components/GPUDashboard/Cards/WorkloadsCard.csssrc/components/GPUDashboard/Cards/WorkloadsCard.tsxsrc/components/GPUDashboard/GPUDashboard.tsxsrc/components/GPUDashboard/GPUSelector.tsxsrc/components/NotAvailable.tsxsrc/components/PodStatus/PodStatus.tsxsrc/components/shared/OverviewDetailItem.csssrc/components/shared/OverviewDetailItem.tsxsrc/utils/units.tstsconfig.jsonwebpack.config.ts
💤 Files with no reviewable changes (2)
- .prettierrc.yml
- .eslintrc
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (16)
- src/components/PodStatus/PodStatus.tsx
- .stylelintrc.yaml
- src/components/shared/OverviewDetailItem.css
- .eslintrc.yml
- src/components/shared/OverviewDetailItem.tsx
- src/components/GPUDashboard/GPUSelector.tsx
- README.md
- src/components/GPUDashboard/Cards/WorkloadsCard.tsx
- src/components/ClusterOverview/GPUProviders.tsx
- i18n-scripts/common.js
- i18n-scripts/build-i18n.sh
- i18n-scripts/lexers.js
- src/components/GPUDashboard/Cards/WorkloadsCard.css
- Dockerfile
- src/components/GPUDashboard/GPUDashboard.tsx
- tsconfig.json
🔇 Additional comments (18)
src/components/GPUDashboard/Cards/MetricsCard.tsx (1)
95-95: LGTM!The CSS class update from
pf-u-text-align-centertopf-v6-u-text-align-centercorrectly follows the PatternFly v6 utility class naming convention.src/components/NotAvailable.tsx (1)
8-8: LGTM!The CSS class update from
pf-u-color-200topf-v6-u-color-200correctly follows the PatternFly v6 utility class naming convention, consistent with the broader PF v6 migration in this PR.deployment/console-plugin-nvidia-gpu/Chart.yaml (1)
4-4: LGTM!The description update to require OpenShift 4.19+ and the version bump to 0.3.0 are consistent with the PR objectives. The minor version increment is appropriate given the significant dependency upgrades (Console SDK 1.4.0, PatternFly v6, Node.js 20) that necessitate the new OpenShift minimum version.
Also applies to: 15-15
src/components/GPUDashboard/Cards/InfoCard.tsx (1)
3-3: LGTM! Import path updated to use local shared component.This change correctly migrates from the external
@openshift-console/plugin-sharedpackage to the local shared module, which aligns with the Console SDK upgrade. The localOverviewDetailItemcomponent exports the correct API with all required props (title,isLoading,error,children), and the component usage throughout the file is fully compatible.src/components/ClusterOverview/GPUStatus.tsx (2)
112-116: LGTM!The icon color assignments correctly reference the
okColorstring directly. This aligns with the change from the previous JS token object (which required.value) to a CSS variable string.Also applies to: 147-151
18-19: CSS variable token is correct for PatternFly v6.The CSS token
--pf-t--global--color--status--success--defaultis a valid PatternFly v6 semantic color token, and the fallback value#3D7317appropriately provides a green success color. The migration from PF4 JS tokens to PF6 CSS variables is properly implemented.src/utils/units.ts (4)
179-187: LGTM: Type signature expansion for SDK compatibility.Accepting
string | numberaligns with the SDK'sHumanizetype and provides flexibility for callers passing metric values that may arrive as strings.
189-208: LGTM: Consistent numeric coercion across wrapper functions.
Number(v)correctly handles both string and number inputs, and NaN from invalid strings is handled downstream byhumanize()which defaults non-finite values to 0.
220-233: LGTM: Proper NaN and edge-case handling.The explicit check for non-finite values and
-0normalization ensures consistent behavior. Bothstringandvaluefields will reflect the same normalized value.
235-246: LGTM: Consistent withhumanizeRatiopattern.Same robust handling of non-finite values. The division by 100 correctly converts percentage input to ratio for
formatPercentage.i18next-parser.config.js (1)
1-31: LGTM! Well-structured i18next-parser configuration.The configuration properly sets up lexers for all relevant file types and integrates the custom JSON lexer. The
sort: trueoption ensures consistent, deterministic output which aids in diffing locale files.webpack.config.ts (2)
61-73: Note:allowedHosts: 'all'allows any host to connect to the dev server.This is generally acceptable for local development, but ensure this configuration is never used in production-facing environments. The current setup is development-only given it's within
devServer, which is appropriate.Verify this dev server is only used locally and not exposed to external networks in your development workflow.
16-84: LGTM! Clean production-aware webpack configuration.The configuration properly handles:
- Production/development mode switching
- Content hashing for cache busting in production
- SCSS support with the loader chain
- ESM module resolution with
fullySpecified: false- Deterministic chunk IDs for consistent builds
i18n-scripts/set-english-defaults.js (1)
68-81: LGTM! Clean async execution with proper error handling.The script properly filters for the 'en' locale directory and includes appropriate error handling with a non-zero exit code on failure.
package.json (4)
5-8: LGTM! Clear license and engine requirements.Good addition of the Apache-2.0 license and Node.js >=20.0.0 engine requirement, aligning with the PR objective to upgrade to Node.js 20.
9-20: LGTM! Well-structured build scripts.The scripts properly:
- Set
NODE_ENV=productionfor production builds- Use
ts-node/registerto handle the TypeScript webpack config- Chain the i18n extraction and processing steps
83-86: LGTM! Appropriate peer dependency declaration.Declaring
i18nextas a peer dependency correctly communicates to the host OpenShift Console that this runtime dependency should be provided by the host application.
21-64: Verify PatternFly and React version compatibility.The configuration uses:
@patternfly/react-charts: ^7.4.2@patternfly/react-core: ^6.2.2react: ^17.0.1PatternFly v6 typically targets React 18. Verify that these versions work correctly together with React 17.
PatternFly 6 React 17 compatibility requirements
empovit
force-pushed
the
update-for-ocp-4.19-no-chart-changes
branch
from
3fc937c to
0bd1259
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @i18n-scripts/set-english-defaults.js:
- Around line 21-27: The updateFile function currently uses require(fileName)
which is cached by Node and yields stale JSON on subsequent runs; replace
require with a synchronous file read and parse (e.g., fs.readFileSync(fileName,
'utf8') followed by JSON.parse) to always reload the file, handle and rethrow
parsing or read errors with a clear message including fileName, and ensure any
downstream code expecting the object continues to receive the parsed object from
updateFile.
🧹 Nitpick comments (4)
src/components/shared/OverviewDetailItem.tsx (1)
14-36: Clean component implementation with proper state handling.The loading → error → children priority is sensible. One optional simplification for the className on line 33:
♻️ Optional: Simplify className construction
- <dd className={valueClassName ? `ng-overview-detail-item__value ${valueClassName}` : 'ng-overview-detail-item__value'}>{value}</dd> + <dd className={`ng-overview-detail-item__value${valueClassName ? ` ${valueClassName}` : ''}`}>{value}</dd>Alternatively, consider using a utility like
clsxif the project already has it as a dependency for more complex class concatenation scenarios.src/utils/units.ts (1)
209-221: Consider zero-value display.When
numValueis 0 (or coerced to 0 from non-finite input), the conditionnumValue < 1is true, resulting in unit'm'and displaying "0m" instead of "0". While "0m" (zero millicores) is technically valid in Kubernetes terminology, displaying plain "0" might be cleaner for zero values.💡 Optional fix to display "0" for zero values
export const humanizeCpuCores: HumanizeFunc = (v) => { let numValue = Number(v); if (!isFinite(numValue) || numValue === 0) { numValue = 0; } - const value = numValue < 1 ? round(numValue * 1000) : numValue; - const unit = numValue < 1 ? 'm' : ''; + const value = numValue < 1 && numValue > 0 ? round(numValue * 1000) : numValue; + const unit = numValue < 1 && numValue > 0 ? 'm' : ''; return { string: `${formatValue(value)}${unit}`, unit, value, }; };i18n-scripts/common.js (1)
14-26: Consider simplifying the async IIFE pattern.The async IIFE wrapper is unnecessary. An
asyncfunction already returns a Promise, so you can declareparseFolderas async directly.♻️ Suggested simplification
- parseFolder(directory, argFunction, packageDir) { - return (async () => { - try { - const files = await fs.promises.readdir(directory); - for (const file of files) { - const filePath = path.join(directory, file); - await argFunction(filePath, packageDir); - } - } catch (e) { - console.error(`Failed to parseFolder ${directory}:`, e); - throw e; - } - })(); - }, + async parseFolder(directory, argFunction, packageDir) { + try { + const files = await fs.promises.readdir(directory); + for (const file of files) { + const filePath = path.join(directory, file); + await argFunction(filePath, packageDir); + } + } catch (e) { + console.error(`Failed to parseFolder ${directory}:`, e); + throw e; + } + },i18n-scripts/set-english-defaults.js (1)
8-19: Consider usingendsWithfor more precise suffix matching.The current
includes()checks could match keys where the suffix appears mid-string (e.g.,WithCount_pluralExtra). While unlikely given i18next conventions, usingendsWith()would be more precise.Suggested refinement
function determineRule(key) { - if (key.includes('WithCount_plural')) { + if (key.endsWith('WithCount_plural')) { return 0; } - if (key.includes('WithCount')) { + if (key.endsWith('WithCount')) { return 1; } - if (key.includes('_plural')) { + if (key.endsWith('_plural')) { return 2; } return 3; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (30)
.dockerignore.eslintrc.eslintrc.yml.gitignore.prettierrc.yml.stylelintrc.yamlDockerfileREADME.mddeployment/console-plugin-nvidia-gpu/Chart.yamli18n-scripts/build-i18n.shi18n-scripts/common.jsi18n-scripts/lexers.jsi18n-scripts/set-english-defaults.jsi18next-parser.config.jspackage.jsonsrc/components/ClusterOverview/GPUProviders.tsxsrc/components/ClusterOverview/GPUStatus.tsxsrc/components/GPUDashboard/Cards/InfoCard.tsxsrc/components/GPUDashboard/Cards/MetricsCard.tsxsrc/components/GPUDashboard/Cards/WorkloadsCard.csssrc/components/GPUDashboard/Cards/WorkloadsCard.tsxsrc/components/GPUDashboard/GPUDashboard.tsxsrc/components/GPUDashboard/GPUSelector.tsxsrc/components/NotAvailable.tsxsrc/components/PodStatus/PodStatus.tsxsrc/components/shared/OverviewDetailItem.csssrc/components/shared/OverviewDetailItem.tsxsrc/utils/units.tstsconfig.jsonwebpack.config.ts
💤 Files with no reviewable changes (2)
- .eslintrc
- .prettierrc.yml
🚧 Files skipped from review as they are similar to previous changes (16)
- src/components/ClusterOverview/GPUStatus.tsx
- src/components/GPUDashboard/GPUDashboard.tsx
- src/components/GPUDashboard/Cards/WorkloadsCard.tsx
- tsconfig.json
- src/components/NotAvailable.tsx
- src/components/GPUDashboard/Cards/InfoCard.tsx
- i18n-scripts/lexers.js
- src/components/GPUDashboard/GPUSelector.tsx
- .stylelintrc.yaml
- src/components/GPUDashboard/Cards/WorkloadsCard.css
- i18next-parser.config.js
- src/components/ClusterOverview/GPUProviders.tsx
- .gitignore
- src/components/GPUDashboard/Cards/MetricsCard.tsx
- deployment/console-plugin-nvidia-gpu/Chart.yaml
- README.md
🧰 Additional context used
🧬 Code graph analysis (1)
i18n-scripts/common.js (2)
i18n-scripts/set-english-defaults.js (3)
fs(1-1)path(2-2)file(22-22)i18next-parser.config.js (1)
require(2-2)
🔇 Additional comments (23)
src/components/shared/OverviewDetailItem.css (1)
1-18: LGTM!The CSS follows the project's
ng-prefix convention, uses PatternFly v6 CSS variables consistently, and employs good defensive styling withword-breakandoverflow-wrapfor handling long content in values.src/components/shared/OverviewDetailItem.tsx (1)
1-12: LGTM!Clean imports and well-defined props interface. Using semantic
dl/dt/ddelements is appropriate for a definition list-style component and benefits accessibility.src/utils/units.ts (4)
179-187: LGTM!The type signature change to accept
string | numberis backward-compatible and aligns with common patterns where metric values may arrive as strings from APIs or form inputs.
189-208: LGTM!Consistent application of
Number(v)coercion across all wrapper functions. The downstreamhumanizefunction already handles non-finite values gracefully.
223-236: LGTM!The type change to
Humanize(from SDK) aligns with the PR objective to upgrade to Console SDK 1.4.0. The numeric coercion and formatting logic is correct—ratio values are properly converted to percentage display viaformatPercentage.
238-249: LGTM!Consistent with the pattern in
humanizeRatio. The division by 100 before callingformatPercentagecorrectly handles the input being a percentage value (e.g., 50) rather than a ratio (e.g., 0.5), leveragingIntl.NumberFormat's percent style.src/components/PodStatus/PodStatus.tsx (2)
3-3: LGTM!The import correctly uses
Contentfrom PatternFly v6, replacing the deprecatedTextContent,Text, andTextVariantsfrom v4.
116-149: PatternFly v6 migration looks correct.The restructuring properly migrates from PatternFly v4 to v6:
- Fragment wrapper replaces
TextContentappropriately<Content component="p">is the correct v6 replacement for<Text>- Link paths consistently use
pod.metadata.name.dockerignore (1)
1-4: LGTM!Standard .dockerignore patterns that correctly exclude build artifacts, dependencies, and version control from the Docker build context. This keeps the context lean and aligns with the Dockerfile's
yarn install --frozen-lockfile && yarn buildworkflow.i18n-scripts/build-i18n.sh (1)
1-7: LGTM!The script correctly uses strict shell options (
set -exuo pipefail) for fail-fast behavior, and the escaped\$LOCALEand\$NAMESPACEare proper i18next-parser template placeholders..eslintrc.yml (1)
1-25: LGTM!Well-structured ESLint configuration for TypeScript and React with Prettier integration. Disabling
react/prop-typesis appropriate since TypeScript handles type checking, and auto-detecting the React version avoids version mismatches.i18n-scripts/common.js (2)
5-12: LGTM!The
isDirectoryfunction correctly useslstatSyncwith appropriate error handling, returningfalsefor non-existent paths.
28-35: LGTM!The
deleteFilefunction has proper error handling with logging and re-throwing to preserve the error for callers.Dockerfile (1)
1-13: LGTM. The Dockerfile correctly upgrades to Node.js 20 on UBI 9, uses a conditional yarn installation pattern, and maintains proper security by running as non-root (USER 1001) in the final stage. Both base images are available and the build stage path changes are consistent throughout.webpack.config.ts (4)
61-73:allowedHosts: 'all'exposes the dev server to potential DNS rebinding attacks.When the dev server is accessible beyond localhost (e.g., via container port forwarding or network exposure),
allowedHosts: 'all'disables host header checks, which could allow malicious sites to interact with your dev server.If the dev server is only used locally, this is acceptable. If it may be exposed on a network (common in container/OpenShift development workflows), consider restricting to specific hosts or using
allowedHosts: 'auto'.Is this dev server ever exposed beyond localhost in your development workflow (e.g., via OpenShift routes or port forwarding)?
7-8: CommonJS require for copy-webpack-plugin is appropriate for v6.x.The
copy-webpack-pluginv6.x exports a CommonJS module, making the require syntax with the eslint-disable comment the correct approach here.Also applies to: 76-78
16-24: Production/development build configuration looks solid.The environment-aware configuration properly differentiates between production (hashed filenames, minification, deterministic chunk IDs, no source maps) and development (readable names, source maps, named chunks). This aligns with webpack best practices.
Also applies to: 80-84
53-58: ESM compatibility rule is correctly configured.Setting
fullySpecified: falsefor.mjsand.jsfiles resolves ESM import issues where packages don't include file extensions. This is commonly needed for modern ESM-aware packages.i18n-scripts/set-english-defaults.js (1)
74-81: Entry point has proper error handling.The async IIFE correctly catches errors, logs them, and exits with a non-zero code to fail the build pipeline on errors.
package.json (4)
5-8: License and engine specifications are good additions.Explicitly declaring the Apache-2.0 license and Node.js 20+ requirement improves project clarity and prevents runtime environment mismatches.
9-19: Build scripts are cleaner with the unified webpack approach.The refactored scripts properly set
NODE_ENV=productionfor production builds and use a consistent webpack invocation pattern via ts-node for TypeScript config support.
42-42: Remove version compatibility warning—react-i18next 11.x is compatible with i18next 23.x.react-i18next 11.x officially supports i18next >= 10.0.0, and the project's i18next ^23.11.5 satisfies this requirement. The peerDependencies section explicitly declares this pairing, confirming it is an intentional and compatible configuration.
Likely an incorrect or invalid review comment.
24-27: > Likely an incorrect or invalid review comment.
empovit
force-pushed
the
update-for-ocp-4.19-no-chart-changes
branch
from
0bd1259 to
c1d8515
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @src/components/ClusterOverview/GPUStatus.tsx:
- Around line 18-19: The fallback hex for the success token is wrong; update the
okColor constant so its CSS var fallback matches PatternFly v6 by replacing the
hardcoded fallback #3D7317 with #3e8635 (i.e., change the value of the okColor
variable to use 'var(--pf-t--global--color--status--success--default,
#3e8635)').
In @src/utils/units.ts:
- Around line 209-221: In humanizeCpuCores, when numValue is 0 the current logic
treats it as <1 and sets unit='m' producing "0m"; change the conditional so zero
is handled explicitly: if numValue === 0 set value = 0 and unit = '' (and string
= `${formatValue(0)}`) before the numValue < 1 branch, ensuring invalid inputs
coerced to 0 also produce "0" not "0m"; update the return to use these adjusted
value/unit variables in humanizeCpuCores.
🧹 Nitpick comments (2)
src/utils/units.ts (1)
251-261: Consider aligninghumanizeGPUandhumanizeDegreeswith the new coercion pattern.These functions use
Number.parseInt()for the numeric value but template literals for the string, which could produce inconsistent results (e.g., input"3.5"→ string"3.5", value3).Since these weren't modified in this PR, this is optional follow-up work to maintain consistency with the other humanize functions.
package.json (1)
35-35: Consider upgradingcopy-webpack-pluginin a follow-up.Version
^6.4.1is quite dated (current stable is 12.x). While functional, newer versions offer better webpack 5 integration and security patches. This could be addressed in a separate PR to keep this upgrade focused.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (30)
.dockerignore.eslintrc.eslintrc.yml.gitignore.prettierrc.yml.stylelintrc.yamlDockerfileREADME.mddeployment/console-plugin-nvidia-gpu/Chart.yamli18n-scripts/build-i18n.shi18n-scripts/common.jsi18n-scripts/lexers.jsi18n-scripts/set-english-defaults.jsi18next-parser.config.jspackage.jsonsrc/components/ClusterOverview/GPUProviders.tsxsrc/components/ClusterOverview/GPUStatus.tsxsrc/components/GPUDashboard/Cards/InfoCard.tsxsrc/components/GPUDashboard/Cards/MetricsCard.tsxsrc/components/GPUDashboard/Cards/WorkloadsCard.csssrc/components/GPUDashboard/Cards/WorkloadsCard.tsxsrc/components/GPUDashboard/GPUDashboard.tsxsrc/components/GPUDashboard/GPUSelector.tsxsrc/components/NotAvailable.tsxsrc/components/PodStatus/PodStatus.tsxsrc/components/shared/OverviewDetailItem.csssrc/components/shared/OverviewDetailItem.tsxsrc/utils/units.tstsconfig.jsonwebpack.config.ts
💤 Files with no reviewable changes (2)
- .prettierrc.yml
- .eslintrc
🚧 Files skipped from review as they are similar to previous changes (19)
- src/components/GPUDashboard/Cards/MetricsCard.tsx
- tsconfig.json
- src/components/GPUDashboard/GPUDashboard.tsx
- .dockerignore
- i18n-scripts/lexers.js
- src/components/shared/OverviewDetailItem.tsx
- README.md
- i18n-scripts/common.js
- src/components/GPUDashboard/Cards/WorkloadsCard.css
- i18n-scripts/set-english-defaults.js
- .gitignore
- src/components/GPUDashboard/GPUSelector.tsx
- i18n-scripts/build-i18n.sh
- src/components/shared/OverviewDetailItem.css
- src/components/GPUDashboard/Cards/WorkloadsCard.tsx
- .stylelintrc.yaml
- src/components/NotAvailable.tsx
- deployment/console-plugin-nvidia-gpu/Chart.yaml
- Dockerfile
🔇 Additional comments (14)
src/components/ClusterOverview/GPUProviders.tsx (1)
2-2: LGTM!The import path correctly points to the new local
OverviewDetailItemcomponent at../shared/OverviewDetailItem, which aligns with the PR's approach of creating shared UI components to support the Console SDK and PatternFly upgrades.src/components/GPUDashboard/Cards/InfoCard.tsx (1)
3-3: LGTM - Import migrated to local shared component.The import path change from
@openshift-console/plugin-sharedto the local../../shared/OverviewDetailItemaligns with the Console SDK 1.4.0 upgrade. The component usage throughout the file remains consistent with the expected props interface (title,isLoading,error, children).src/components/PodStatus/PodStatus.tsx (2)
3-3: LGTM - PatternFly v6 import migration.The
Contentcomponent correctly replaces the deprecatedTextContent/Textimports for PatternFly v6.
116-149: LGTM - PatternFly v6 Content component migration and URL fixes.The migration from
TextContent/TexttoContent component="p"follows PatternFly v6 patterns correctly. The URL paths at lines 126, 130, and 140 correctly referencepod.metadata.name.src/utils/units.ts (4)
179-187: LGTM!The type change to accept
string | numberaligns with common API response patterns where numeric values may arrive as strings. This improves flexibility while maintaining type safety.
189-208: LGTM!Consistent use of
Number(v)across all wrapper functions. Invalid inputs resulting inNaNare safely handled by the downstreamhumanize()function's!isFinite(value)check.
223-236: LGTM!The ratio-to-percentage conversion logic is correct. The
Humanizetype from the SDK provides better type alignment with the OpenShift Console API.
238-249: LGTM!Correct percentage handling with proper division by 100 before formatting with
Intl.NumberFormat's percent style..eslintrc.yml (1)
1-25: LGTM! Well-structured ESLint configuration.The configuration properly chains extends (with
prettierlast to avoid conflicts), enables JSX support, and disablesreact/prop-typessince TypeScript handles type checking. Auto-detecting the React version is a good practice.i18next-parser.config.js (1)
1-30: LGTM! Comprehensive lexer configuration for i18n extraction.The configuration properly maps file extensions to appropriate lexers. Note that
CustomJSONLexer(line 27) is correctly passed as a class reference rather than a string, since it's a custom implementation imported from the locali18n-scripts/lexersmodule.webpack.config.ts (2)
61-73: VerifyallowedHosts: 'all'is acceptable for your environment.Setting
allowedHosts: 'all'disables host checking entirely, which is convenient for development but could expose the dev server to DNS rebinding attacks if run on a network accessible to untrusted clients. This is generally acceptable for local development only.
74-84: LGTM! Clean production-aware configuration.The webpack config properly differentiates between production and development modes with appropriate optimizations:
- Hash-based filenames for cache busting in production
- Source maps only in development
- Deterministic chunk IDs for production, named for debugging in development
package.json (1)
5-19: LGTM! Scripts and engine constraints are well-defined.The
enginesfield properly enforces Node.js 20+ as stated in PR objectives. The refactored scripts cleanly separate concerns:
webpackscript abstracts the ts-node registrationbuild/build-devuse the shared webpack script with appropriate NODE_ENVtype-checkenables standalone TypeScript validationsrc/components/ClusterOverview/GPUStatus.tsx (1)
115-116: LGTM!The
okColorCSS variable is correctly applied to theCheckCircleIconfor both temperature and operator healthy states. Thecolorprop accepts CSS variable strings, so this will resolve appropriately at runtime.Also applies to: 150-151
empovit
force-pushed
the
update-for-ocp-4.19-no-chart-changes
branch
from
c1d8515 to
51c82b8
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @package.json:
- Line 61: Update the webpack dependency in package.json from "5.75.0" to a
secure, patched version (preferably "5.94.0" or at minimum "5.76.0") and change
the version spec to a caret range (e.g., ^5.94.0) so future security patches are
picked up; after updating the "webpack" entry, run your package manager
(npm/yarn) to update lockfile (package-lock.json or yarn.lock) and run the
test/build pipeline to verify there are no breakages.
- Around line 42-43: package.json lists "i18next" at ^23.11.5 but
"i18next-parser" is pinned to the old 3.x series; update the dependency entry
for "i18next-parser" to a 9.3.0+ spec (for example "^9.3.0"), run your package
manager install (npm/yarn/pnpm) to refresh lockfiles, and run the i18next-parser
build/extraction to catch any config/api breaking changes — if extraction fails
adjust the i18next-parser configuration (e.g., keysSeparator/namespace handling)
to match the new parser API used by the updated "i18next-parser".
In @src/components/shared/OverviewDetailItem.tsx:
- Around line 24-25: The span rendering error text in OverviewDetailItem.tsx
currently uses a non-existent PatternFly v6 utility class; replace that by
adding a CSS rule in OverviewDetailItem.css that sets color to the PatternFly
danger token (var(--pf-t--global--text--color--status--danger--default)) and
then change the span in OverviewDetailItem.tsx to use a descriptive className
(e.g., ng-overview-detail-item__error) so the error text uses the design token
instead of the invalid utility class.
In @src/utils/units.ts:
- Around line 209-221: humanizeCpuCores currently treats numValue < 1 as
millicores so a zero input yields "0m"; add an explicit check at the start of
humanizeCpuCores for numValue === 0 and return { string: `${formatValue(0)}`,
unit: '', value: 0 } (or equivalent) to ensure zero displays as "0" (use the
existing symbols numValue, value, unit, round, formatValue and preserve the rest
of the logic for values <1 and >=1).
🧹 Nitpick comments (2)
webpack.config.ts (1)
61-73: Security consideration for dev server configuration.
allowedHosts: 'all'disables host header validation, which can expose the dev server to DNS rebinding attacks. This is acceptable for local development but ensure this configuration is never used in any exposed environment.Consider documenting this as a dev-only setting or restricting to localhost in comments:
devServer: { static: './dist', port: 9001, - allowedHosts: 'all', + // WARNING: 'all' disables host checking - for local development only + allowedHosts: 'all', headers: {src/components/GPUDashboard/GPUSelector.tsx (1)
47-56: Consider displaying a human-readable GPU identifier instead of the raw UUID.The toggle displays
selectedGPU?.uuidwhich shows a raw UUID to users. For better UX, consider showing the model name or a combination of model and a short identifier.💡 Suggested improvement
<MenuToggle ref={toggleRef} onClick={() => setOpen(!isOpen)} isExpanded={isOpen} isDisabled={isDisabled} > - {selectedGPU?.uuid || t('Select GPU')} + {selectedGPU?.modelName || selectedGPU?.uuid || t('Select GPU')} </MenuToggle>
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (30)
.dockerignore.eslintrc.eslintrc.yml.gitignore.prettierrc.yml.stylelintrc.yamlDockerfileREADME.mddeployment/console-plugin-nvidia-gpu/Chart.yamli18n-scripts/build-i18n.shi18n-scripts/common.jsi18n-scripts/lexers.jsi18n-scripts/set-english-defaults.jsi18next-parser.config.jspackage.jsonsrc/components/ClusterOverview/GPUProviders.tsxsrc/components/ClusterOverview/GPUStatus.tsxsrc/components/GPUDashboard/Cards/InfoCard.tsxsrc/components/GPUDashboard/Cards/MetricsCard.tsxsrc/components/GPUDashboard/Cards/WorkloadsCard.csssrc/components/GPUDashboard/Cards/WorkloadsCard.tsxsrc/components/GPUDashboard/GPUDashboard.tsxsrc/components/GPUDashboard/GPUSelector.tsxsrc/components/NotAvailable.tsxsrc/components/PodStatus/PodStatus.tsxsrc/components/shared/OverviewDetailItem.csssrc/components/shared/OverviewDetailItem.tsxsrc/utils/units.tstsconfig.jsonwebpack.config.ts
💤 Files with no reviewable changes (2)
- .prettierrc.yml
- .eslintrc
🚧 Files skipped from review as they are similar to previous changes (12)
- src/components/NotAvailable.tsx
- src/components/GPUDashboard/Cards/MetricsCard.tsx
- src/components/PodStatus/PodStatus.tsx
- i18n-scripts/common.js
- i18n-scripts/set-english-defaults.js
- src/components/ClusterOverview/GPUProviders.tsx
- .gitignore
- src/components/GPUDashboard/Cards/WorkloadsCard.css
- tsconfig.json
- src/components/ClusterOverview/GPUStatus.tsx
- README.md
- deployment/console-plugin-nvidia-gpu/Chart.yaml
🧰 Additional context used
🧬 Code graph analysis (2)
i18n-scripts/lexers.js (2)
i18next-parser.config.js (1)
require(2-2)i18n-scripts/set-english-defaults.js (1)
keys(35-35)
src/components/GPUDashboard/GPUDashboard.tsx (1)
src/components/GPUDashboard/Cards/GPUDashboardGraphs.tsx (1)
GPUDashboardGraphs(48-146)
🔇 Additional comments (21)
src/utils/units.ts (4)
179-187: LGTM!The type signature update to accept
string | numberis appropriate for handling inputs that may arrive as strings (e.g., from JSON or query parameters), with internal coercion ensuring numeric processing.
189-208: LGTM!Consistent application of
Number(v)coercion across all wrapper functions ensures robust handling of mixed input types while maintaining the existing numeric processing logic.
238-249: LGTM!The implementation correctly handles percentage values with proper coercion and formatting. The division by 100 before
formatPercentageis necessary sinceIntl.NumberFormatwithstyle: 'percent'multiplies by 100 internally.
223-236: LGTM!The logic correctly handles ratio-to-percentage conversion with appropriate numeric coercion and edge case handling. The function properly implements the SDK's
Humanizetype signature—it accepts a value parameter and returns the expectedHumanizeResultobject with string, unit, and value properties..dockerignore (1)
1-4: LGTM!Standard ignore patterns that correctly exclude build artifacts, dependencies, version control, and logs from the Docker build context. This reduces context size and ensures clean builds.
.eslintrc.yml (1)
1-27: LGTM!Well-structured ESLint configuration for TypeScript + React with Prettier integration. Disabling
react/prop-typesis appropriate since TypeScript handles type checking. Thereact.version: detectsetting is a good practice.Minor: Two trailing blank lines at end of file (lines 26-27) could be reduced to one.
Dockerfile (1)
1-13: LGTM!Good upgrade to Node.js 20 on UBI9, aligning with the PR objectives. The build layout is clean with consistent paths, and the final image correctly runs as non-root user (1001). The multi-stage build keeps the final image lean by only copying the dist artifacts.
.stylelintrc.yaml (1)
1-28: LGTM!Well-documented Stylelint configuration that enforces PatternFly best practices. Key decisions are sound:
- Disallowing named/hex colors and
rgb()ensures dark mode compatibility via PF global variables- Protecting
.pf-and.co-prefixes prevents conflicts with PatternFly and OpenShift console- Allowing BEM-style classnames while restricting bare element selectors is appropriate for plugin development
webpack.config.ts (2)
16-24: LGTM!Clean production/development mode handling with appropriate settings:
- Hash-based filenames in production for cache busting
- Source maps disabled in production, enabled in development
- Deterministic chunk IDs in production for long-term caching
- Minimization correctly gated behind
isProdAlso applies to: 80-85
74-79: The locales directory exists and contains the expected i18n files. No action needed.src/components/GPUDashboard/Cards/InfoCard.tsx (1)
3-3: LGTM! Good migration to local component.Moving
OverviewDetailItemfrom the external@openshift-console/plugin-sharedto a local shared module is a sensible approach for the Console SDK upgrade. This provides better control over the component's behavior and styling while reducing external dependencies.src/components/GPUDashboard/Cards/WorkloadsCard.tsx (2)
48-55: LGTM! Consistent CSS class naming convention.The rename from
co-break-wordtong-break-wordcorrectly follows the template CSS naming conventions (ng-prefix) mentioned in the PR objectives. The change is applied consistently across all table columns.
42-45: The EmptyState API usage is correct for PatternFly v6. The icon prop properly accepts the CubesIcon component, and EmptyStateBody as a child element is valid. This is the correct and only way to implement empty states in PatternFly v6, as EmptyStateIcon is now internal to the component.src/components/GPUDashboard/GPUDashboard.tsx (1)
26-55: LGTM! Clean layout restructuring.The refactored layout using
Stack,Split, andGridfollows PatternFly v6 best practices:
PageSectiondirectly without aPagewrapper simplifies the component hierarchySplitwithisFilledcorrectly allocates space between title and selectorGridwithspan={6}provides equal width forMIGCardandInfoCardStackwithhasGutterensures consistent vertical spacingThe structure is readable and maintainable.
i18n-scripts/lexers.js (1)
7-29: LGTM! Well-implemented custom lexer.The
CustomJSONLexercorrectly:
- Extends
EventEmitter(required by i18next-parser's lexer interface)- Uses
comment-jsonto handle JSON with comments- Extracts keys matching the
%key%pattern via the reviver callback- Provides proper error handling with filename context
The implementation is clean and follows the expected lexer contract.
i18n-scripts/build-i18n.sh (1)
1-7: Script correctly uses strict mode and glob pattern handling.The script properly uses
set -exuo pipefailfor fail-fast behavior. TheFILE_PATTERNin single quotes is correctly passed to i18next-parser as a literal string, allowing the tool's internal glob library to process the pattern—this is the right approach rather than shell expansion. The output filelocales/en/plugin__console-plugin-nvidia-gpu.jsonconfirms the script executes successfully with the configured pattern.src/components/shared/OverviewDetailItem.css (1)
1-18: LGTM! Well-structured CSS using PatternFly v6 design tokens.The CSS follows the
ng-prefix convention as intended, uses appropriate PF6 design tokens (--pf-t--global--*), and theword-break/overflow-wrapon the value class properly handles long text overflow.i18next-parser.config.js (1)
15-30: LGTM! Lexers configuration is properly structured.The configuration correctly distinguishes between built-in lexers (referenced by string names like
'JavascriptLexer') and the customCustomJSONLexer(passed as a class reference). This is the expected pattern for i18next-parser custom lexer integration.src/components/GPUDashboard/GPUSelector.tsx (1)
39-59: PatternFly 6 Select implementation looks correct.The migration to PF6's controlled Select with
MenuTogglepattern is properly implemented. State management viaisOpen/onOpenChangeand selection handling viaonSelectfollow the expected PF6 patterns.src/components/shared/OverviewDetailItem.tsx (1)
14-36: Well-structured reusable component.The component properly handles loading, error, and content states with appropriate semantic HTML (
dl/dt/dd). The props interface is clean and the conditional rendering logic is straightforward.package.json (1)
84-86: Appropriate use of peerDependencies for i18next.Adding
i18nextas a peer dependency is correct for OpenShift Console plugins, as the host console provides the i18next instance. This ensures version compatibility without bundling a duplicate.
empovit
force-pushed
the
update-for-ocp-4.19-no-chart-changes
branch
2 times, most recently
from
f2d0c5d to
58fbe0d
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @locales/en/plugin__console-plugin-nvidia-gpu.json:
- Around line 2-5: The pluralization entries mistakenly include the i18next key
suffixes in their values (e.g., "{{count}} GPU pod_one"); update the values for
the keys "{{count}} GPU pod_one", "{{count}} GPU pod_other", "{{count}} Graphics
Cards_one", and "{{count}} Graphics Cards_other" to remove the "_one"/"_other"
suffixes so they contain only the proper display text with interpolation (for
example "{{count}} GPU pod" for the _one form and "{{count}} GPU pods" for the
_other form, and similarly "Graphics Card"/"Graphics Cards").
In @package.json:
- Line 61: The package.json currently pins "webpack": "5.75.0", which is
vulnerable; update the webpack dependency to at least "5.76.0" (or "5.94.0" to
also address the DOM-clobbering XSS) by changing the "webpack" version string in
package.json and then run your package manager install and regenerate lockfile
(npm install or yarn install / pnpm install) to update the lockfile; ensure any
CI or build references that pin webpack are updated to the new version as well.
- Line 35: The package.json entry for copy-webpack-plugin is pinned to "^6.4.1";
either update that dependency to a current compatible release (e.g., the latest
v13.x if your webpack setup supports it) by changing the version string for
"copy-webpack-plugin", or if v6 is intentionally required for compatibility, add
a comment in package.json (or in the repo README/DEPENDENCIES note) explaining
why v6 is pinned and any compatibility constraints; ensure any upgrade is tested
against webpack config and CI to catch breaking changes in copy-webpack-plugin's
major versions.
In @README.md:
- Line 24: Update the compatibility table in the README to add a row documenting
support for OpenShift 4.19+ by the new plugin/chart version 0.3.0: insert an
entry mapping "OpenShift (OCP) 4.19+" (or "4.19+") to the plugin/chart version
"0.3.0" and ensure the table formatting matches existing rows (same columns and
alignment) so the new entry appears next to the other OCP ranges.
🧹 Nitpick comments (6)
src/components/ClusterOverview/GPUProviders.tsx (1)
11-38: Consider simplifying the nested conditionals.The deeply nested if/else structure is harder to read and maintain. A lookup-based approach would be cleaner.
♻️ Optional: Simplify with a lookup pattern
- let result = t('No GPUs detected'); - if (providers.nvidia) { - if (providers.amd) { - if (providers.other) { - result = t('NVIDIA, AMD and others'); - } else { - result = t('NVIDIA and AMD'); - } - } else { - if (providers.other) { - result = t('NVIDIA and others'); - } else { - result = t('NVIDIA'); - } - } - } else { - if (providers.amd) { - if (providers.other) { - result = t('AMD and others'); - } else { - result = t('AMD'); - } - } else { - if (providers.other) { - result = t('others'); - } - // else no GPU - } - } + const key = `${providers.nvidia ? 'n' : ''}${providers.amd ? 'a' : ''}${providers.other ? 'o' : ''}`; + const labels: Record<string, string> = { + nao: t('NVIDIA, AMD and others'), + na: t('NVIDIA and AMD'), + no: t('NVIDIA and others'), + n: t('NVIDIA'), + ao: t('AMD and others'), + a: t('AMD'), + o: t('others'), + }; + const result = labels[key] ?? t('No GPUs detected');src/components/shared/OverviewDetailItem.tsx (2)
22-23: Consider adding accessibility attributes to the loading spinner.Screen readers benefit from knowing content is loading. PatternFly's Spinner supports an
aria-labelprop.♻️ Optional: Add aria-label for accessibility
if (isLoading) { - value = <Spinner size="md" />; + value = <Spinner size="md" aria-label="Loading" />; } else if (error) {
33-33: Optional: Simplify className concatenation.Using template literals consistently improves readability.
♻️ Optional: Use template literal
- <dd className={valueClassName ? `ng-overview-detail-item__value ${valueClassName}` : 'ng-overview-detail-item__value'}>{value}</dd> + <dd className={`ng-overview-detail-item__value${valueClassName ? ` ${valueClassName}` : ''}`}>{value}</dd>src/components/PodStatus/PodStatus.tsx (1)
64-67: Pre-existing bug: potential null reference in unchanged code.This is not introduced by this PR, but line 65 accesses
terminated.signalandterminated.exitCodewhenwaitingis truthy (andwaiting.reasonis falsy). However,terminatedcould beundefinedin this branch, causing a runtime error.Consider addressing this in a follow-up:
} else if (waiting && !waiting.reason) { - phase = terminated.signal - ? `Signal:${terminated.signal}` - : `ExitCode:${terminated.exitCode}`; + phase = waiting.message || 'Waiting'; } else if (running && ready) {src/utils/units.ts (1)
209-221: LGTM, minor edge case to consider.The numeric coercion and normalization logic is correct. One minor observation: when the input is 0, the condition
numValue < 1evaluates to true, resulting in"0m"(millicores) rather than"0". This is technically accurate but may differ from user expectations for zero values.If displaying
"0"for zero cores is preferred:🔧 Optional adjustment
export const humanizeCpuCores: HumanizeFunc = (v) => { let numValue = Number(v); if (!isFinite(numValue) || numValue === 0) { numValue = 0; } - const value = numValue < 1 ? round(numValue * 1000) : numValue; - const unit = numValue < 1 ? 'm' : ''; + const value = numValue < 1 && numValue !== 0 ? round(numValue * 1000) : numValue; + const unit = numValue < 1 && numValue !== 0 ? 'm' : ''; return { string: `${formatValue(value)}${unit}`, unit, value, }; };webpack.config.ts (1)
61-73: Consider restrictingallowedHostsfor development security.While
allowedHosts: 'all'is common for local development, it allows any host to connect to the dev server. This is acceptable for isolated development but could be a concern if the dev server is exposed on a network.If the dev server is only used locally, consider using
allowedHosts: ['localhost', '127.0.0.1']instead. Otherwise, ensure documentation warns developers not to expose the dev server externally.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (33)
.dockerignore.eslintrc.eslintrc.yml.gitignore.prettierrc.yml.stylelintrc.yamlDockerfileREADME.mddeployment/console-plugin-nvidia-gpu/Chart.yamli18n-scripts/build-i18n.shi18n-scripts/common.jsi18n-scripts/lexers.jsi18n-scripts/set-english-defaults.jsi18next-parser.config.jslocales/en/plugin__console-plugin-nvidia-gpu.jsonpackage.jsonsrc/components/ClusterOverview/GPUProviders.tsxsrc/components/ClusterOverview/GPUStatus.tsxsrc/components/GPUDashboard/Cards/InfoCard.tsxsrc/components/GPUDashboard/Cards/MetricsCard.csssrc/components/GPUDashboard/Cards/MetricsCard.tsxsrc/components/GPUDashboard/Cards/WorkloadsCard.csssrc/components/GPUDashboard/Cards/WorkloadsCard.tsxsrc/components/GPUDashboard/GPUDashboard.tsxsrc/components/GPUDashboard/GPUSelector.tsxsrc/components/NotAvailable.csssrc/components/NotAvailable.tsxsrc/components/PodStatus/PodStatus.tsxsrc/components/shared/OverviewDetailItem.csssrc/components/shared/OverviewDetailItem.tsxsrc/utils/units.tstsconfig.jsonwebpack.config.ts
💤 Files with no reviewable changes (2)
- .prettierrc.yml
- .eslintrc
🚧 Files skipped from review as they are similar to previous changes (16)
- src/components/shared/OverviewDetailItem.css
- src/components/GPUDashboard/Cards/WorkloadsCard.tsx
- tsconfig.json
- .stylelintrc.yaml
- i18n-scripts/build-i18n.sh
- i18next-parser.config.js
- .gitignore
- i18n-scripts/set-english-defaults.js
- .eslintrc.yml
- src/components/GPUDashboard/Cards/MetricsCard.tsx
- i18n-scripts/common.js
- i18n-scripts/lexers.js
- src/components/GPUDashboard/GPUSelector.tsx
- deployment/console-plugin-nvidia-gpu/Chart.yaml
- src/components/NotAvailable.tsx
- src/components/GPUDashboard/Cards/InfoCard.tsx
🔇 Additional comments (25)
src/components/ClusterOverview/GPUProviders.tsx (1)
2-2: LGTM on the import change.The migration from
@openshift-console/plugin-sharedto the local shared component aligns with the SDK upgrade objectives.src/components/GPUDashboard/Cards/MetricsCard.css (1)
9-11: LGTM!The new
.ng-metrics-card__titleclass follows theng-prefix naming convention and provides centered alignment for card titles.src/components/GPUDashboard/Cards/WorkloadsCard.css (1)
6-9: LGTM!The
.ng-break-wordutility class provides robust text wrapping. Using bothword-break: break-wordandoverflow-wrap: anywhereensures good fallback behavior across browsers.src/components/shared/OverviewDetailItem.tsx (1)
14-36: LGTM on the new shared component.Clean implementation with proper TypeScript types and state handling for loading, error, and content states.
src/components/GPUDashboard/GPUDashboard.tsx (1)
26-55: LGTM on the simplified layout structure.The restructured layout using
Stack,Split, andGridis clean and follows PatternFly v6 patterns well. The 50/50 grid split for MIGCard and InfoCard is appropriate, and thehasGutterprops ensure consistent spacing.src/components/NotAvailable.css (1)
1-3: Verify PatternFly v6 token name.The class follows the
ng-prefix convention as outlined in the PR objectives. The--pf-t--global--text--color--subtletoken is valid in PatternFly v6 and represents the semantic token for subtle text color used for secondary content.src/components/PodStatus/PodStatus.tsx (2)
116-149: LGTM! Footer restructure and URL fixes look correct.The migration to
Content component="p"for paragraph blocks follows PatternFly v6 conventions. The URL paths now correctly usepod.metadata.name(fixing the previous typo mentioned in the summary).
3-3: PatternFly v6 migration from Text/TextContent to Content is correct.The import change aligns with PatternFly v6 patterns. The Content component is the official replacement for Text, TextContent, and TextList components in PatternFly v6, as documented in the upgrade guide.
src/utils/units.ts (4)
189-208: LGTM!The consistent use of
Number(v)coercion across all wrapper functions is a clean approach to handle the updated type signature that accepts both string and number values.
223-236: LGTM!The normalization logic correctly handles edge cases (
-0,NaN,Infinity), and the ratio-to-percentage conversion viaformatPercentageis accurate sinceIntl.NumberFormatwithstyle: 'percent'multiplies by 100 internally.
238-249: LGTM!The conversion chain is correct: percentage input → divide by 100 to get ratio →
formatPercentagewithstyle: 'percent'multiplies back. Edge case normalization is properly handled.
179-187: Verify SDKHumanizetype compatibility.The custom
HumanizeFunctype acceptsstring | number, whilehumanizeRatioandhumanizePercentageare typed as the SDK'sHumanize. Confirm that the SDK's type signature is compatible with the internalNumber()coercion pattern used here.src/components/ClusterOverview/GPUStatus.tsx (2)
115-116: LGTM!The
CheckCircleIconcolor prop correctly accepts the CSS variable string. Both usages are consistent with the PatternFly v6 migration pattern.Also applies to: 150-151
18-19: Verify the fallback color matches PatternFly v6 token resolution.The CSS variable token
--pf-t--global--color--status--success--defaultis correct for PatternFly v6, but according to PatternFly documentation, this semantic token resolves to#3d7317in the light theme, not#3e8635. Confirm whether the fallback color should be updated to#3d7317to match the actual token value, or clarify if#3e8635is intentional.locales/en/plugin__console-plugin-nvidia-gpu.json (1)
6-85: LGTM!The GPU-centric vocabulary reorganization looks well-structured. Keys are alphabetically sorted, terminology is consistent (GPU, Donut, Sparkline prefixes for metric variants), and the new keys align with the dashboard UI changes mentioned in the PR.
.dockerignore (1)
1-4: LGTM!Standard and appropriate exclusions for a Node.js Docker build context. These patterns will reduce build context size and prevent unnecessary files from being copied into the image.
Dockerfile (2)
1-7: LGTM - Node.js 20 upgrade aligns with package.json requirements.The base image upgrade to
ubi9/nodejs-20correctly matches theenginesconstraint inpackage.json. The conditional yarn installation and updated working directory paths are appropriate.
9-13: LGTM!The final stage correctly copies artifacts from the updated build path, and the runtime configuration remains appropriate for the nginx-based serving approach.
webpack.config.ts (4)
7-10: LGTM - Clean production/development toggle.The
isProdflag provides a clean way to toggle between production and development configurations. The CommonJS require forcopy-webpack-pluginwith the eslint disable comment is appropriate given the module's export style.
43-44: LGTM - SCSS support added.The addition of
sass-loaderaligns with thesassdependency inpackage.jsonand enables SCSS file processing alongside CSS.
53-58: LGTM - ESM compatibility fix.Setting
fullySpecified: falsefor.mjs/.jsmodules is a common workaround for ESM packages that don't fully specify their imports, which is often needed when consuming newer dependencies.
76-84: LGTM - Production optimizations.The production configuration correctly enables deterministic chunk IDs (for long-term caching), minimization, and disables source maps. The
localespath resolution usingpath.resolveis more robust than relative paths.package.json (3)
5-8: LGTM - License and engine constraints added.The
Apache-2.0license andnode >= 20.0.0engine constraint align with the PR objectives and Dockerfile base image upgrade.
84-86: Verifyi18nextpeerDependency is appropriate.Adding
i18nextas a peerDependency with the same version as devDependencies (^23.11.5) is correct if the plugin expects the host application to provide i18next at runtime. Ensure this aligns with OpenShift Console's provided dependencies.
22-27: Incorrect SDK version for OpenShift 4.19 compatibility.PatternFly v6 upgrade is correct for OpenShift 4.19 (PF4 support removed). However, the SDK version
1.4.0is for OpenShift Console 4.16.x, not 4.19. For OpenShift 4.19+ compatibility, the SDK version should be4.19.xto match the Console minor version. Update the package.json accordingly.Likely an incorrect or invalid review comment.
empovit
force-pushed
the
update-for-ocp-4.19-no-chart-changes
branch
from
58fbe0d to
20e8d07
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/PodStatus/PodStatus.tsx (1)
64-67: Potential null pointer dereference in waiting state handling.When
waiting && !waiting.reasonis true butterminatedis undefined (which is possible sincewaitingbeing truthy doesn't guaranteeterminatedexists), accessingterminated.signalandterminated.exitCodewill throw a TypeError.This appears to be a pre-existing bug, but it's worth fixing during this refactor.
🐛 Proposed fix
} else if (waiting && !waiting.reason) { - phase = terminated.signal - ? `Signal:${terminated.signal}` - : `ExitCode:${terminated.exitCode}`; + phase = terminated + ? terminated.signal + ? `Signal:${terminated.signal}` + : `ExitCode:${terminated.exitCode}` + : 'Waiting'; } else if (running && ready) {
🤖 Fix all issues with AI agents
In @.eslintrc.yml:
- Around line 1-26: Add a stricter TypeScript ESLint preset to the extends array
(for example "plugin:@typescript-eslint/recommended-requiring-type-checking" or
"plugin:@typescript-eslint/strict") and, if you choose the
requiring-type-checking preset, configure parserOptions.project to point to your
tsconfig (e.g., "parserOptions.project": "./tsconfig.json"); update the extends
list and parserOptions in the existing .eslintrc.yml (refer to the extends and
parserOptions keys) so type-aware rules are enabled and the linter can perform
type-checking.
In @package.json:
- Line 47: Remove the unused "prettier-stylelint" dependency from package.json
by deleting the "prettier-stylelint": "^0.4.2" entry (it may appear under
dependencies or devDependencies), then run your package manager (npm install or
yarn install) to update package-lock.json or yarn.lock accordingly; verify no
scripts reference "prettier-stylelint" and commit the updated package.json and
lockfile.
- Around line 24-27: The @patternfly/react-charts dependency is on v7 (
"^7.4.2") which is incompatible with the PatternFly 6 stack; update the
package.json dependency for "@patternfly/react-charts" to a PatternFly
6–compatible v8 release (e.g. "^8.0.0" or latest v8), then reinstall (npm/yarn
install) and verify peer dependencies resolve without warnings.
🧹 Nitpick comments (5)
src/components/PodStatus/PodStatus.tsx (1)
116-149: PatternFly v6 Content migration looks correct.The migration from
Text/TextContenttoContent component="p"is appropriate for PF v6.One minor improvement: the
mapat line 135 implicitly returnsundefinedfor containers that don't match the condition, resulting in empty array slots. Consider usingfilterbeforemapor returningnullexplicitly.♻️ Optional: Use filter().map() pattern
<Divider /> - {containers.map((container) => { - if (isContainerCrashLoopBackOff(pod, container.name) && !isWindowsPod(pod)) { - return ( - <div key={container.name}> - <Link - to={`/k8s/ns/${pod.metadata.namespace}/pods/${pod.metadata.name}/containers/${container.name}/debug`} - data-test={`popup-debug-container-link-${container.name}`} - > - {t('Debug container {{name}}', { name: container.name })} - </Link> - </div> - ); - } - })} + {containers + .filter((container) => isContainerCrashLoopBackOff(pod, container.name) && !isWindowsPod(pod)) + .map((container) => ( + <div key={container.name}> + <Link + to={`/k8s/ns/${pod.metadata.namespace}/pods/${pod.metadata.name}/containers/${container.name}/debug`} + data-test={`popup-debug-container-link-${container.name}`} + > + {t('Debug container {{name}}', { name: container.name })} + </Link> + </div> + ))}src/components/GPUDashboard/GPUSelector.tsx (1)
47-56: Consider showing GPU model name in toggle for better UX.The toggle currently displays the raw UUID (
selectedGPU?.uuid), which may be long and not user-friendly. Consider displaying the model name instead:💡 Suggested improvement
<MenuToggle ref={toggleRef} onClick={() => setOpen(!isOpen)} isExpanded={isOpen} isDisabled={isDisabled} > - {selectedGPU?.uuid || t('Select GPU')} + {selectedGPU?.modelName || t('Select GPU')} </MenuToggle>src/components/shared/OverviewDetailItem.tsx (1)
30-35: Consider simplifying className logic.The inline ternary on line 33 works but could be cleaner with a template literal or extracted variable:
💅 Optional cleanup
+ const ddClassName = valueClassName + ? `ng-overview-detail-item__value ${valueClassName}` + : 'ng-overview-detail-item__value'; + return ( <dl className="ng-overview-detail-item"> <dt className="ng-overview-detail-item__title">{title}</dt> - <dd className={valueClassName ? `ng-overview-detail-item__value ${valueClassName}` : 'ng-overview-detail-item__value'}>{value}</dd> + <dd className={ddClassName}>{value}</dd> </dl> );i18n-scripts/set-english-defaults.js (1)
8-60: Consider using consistent async/sync file operations.The function mixes synchronous read (
fs.readFileSync) with asynchronous write (fs.promises.writeFile). While functional, this is inconsistent. Since the caller usesawait, consider usingfs.promises.readFilefor consistency.♻️ Optional: Use async read for consistency
-function updateFile(fileName) { - let file; - try { - const fileContent = fs.readFileSync(fileName, 'utf8'); - file = JSON.parse(fileContent); - } catch (err) { - throw new Error(`Failed to read or parse ${fileName}: ${err.message}`); - } +async function updateFile(fileName) { + let file; + try { + const fileContent = await fs.promises.readFile(fileName, 'utf8'); + file = JSON.parse(fileContent); + } catch (err) { + throw new Error(`Failed to read or parse ${fileName}: ${err.message}`); + }src/utils/units.ts (1)
209-221: Minor edge case: Zero cores displays as "0m".When
numValueis 0 (after normalization), the conditionnumValue < 1evaluates to true, resulting inunit = 'm'. This means zero cores will display as"0m"rather than"0".If this is intentional for UI consistency with sub-core values, no action needed. Otherwise, consider:
♻️ Optional: Display zero without millicores unit
export const humanizeCpuCores: HumanizeFunc = (v) => { let numValue = Number(v); if (!isFinite(numValue) || numValue === 0) { numValue = 0; } - const value = numValue < 1 ? round(numValue * 1000) : numValue; - const unit = numValue < 1 ? 'm' : ''; + const value = numValue < 1 && numValue > 0 ? round(numValue * 1000) : numValue; + const unit = numValue < 1 && numValue > 0 ? 'm' : ''; return { string: `${formatValue(value)}${unit}`, unit, value, }; };
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (33)
.dockerignore.eslintrc.eslintrc.yml.gitignore.prettierrc.yml.stylelintrc.yamlDockerfileREADME.mddeployment/console-plugin-nvidia-gpu/Chart.yamli18n-scripts/build-i18n.shi18n-scripts/common.jsi18n-scripts/lexers.jsi18n-scripts/set-english-defaults.jsi18next-parser.config.jslocales/en/plugin__console-plugin-nvidia-gpu.jsonpackage.jsonsrc/components/ClusterOverview/GPUProviders.tsxsrc/components/ClusterOverview/GPUStatus.tsxsrc/components/GPUDashboard/Cards/InfoCard.tsxsrc/components/GPUDashboard/Cards/MetricsCard.csssrc/components/GPUDashboard/Cards/MetricsCard.tsxsrc/components/GPUDashboard/Cards/WorkloadsCard.csssrc/components/GPUDashboard/Cards/WorkloadsCard.tsxsrc/components/GPUDashboard/GPUDashboard.tsxsrc/components/GPUDashboard/GPUSelector.tsxsrc/components/NotAvailable.csssrc/components/NotAvailable.tsxsrc/components/PodStatus/PodStatus.tsxsrc/components/shared/OverviewDetailItem.csssrc/components/shared/OverviewDetailItem.tsxsrc/utils/units.tstsconfig.jsonwebpack.config.ts
💤 Files with no reviewable changes (2)
- .eslintrc
- .prettierrc.yml
🚧 Files skipped from review as they are similar to previous changes (16)
- src/components/NotAvailable.css
- src/components/GPUDashboard/Cards/MetricsCard.css
- src/components/ClusterOverview/GPUProviders.tsx
- src/components/GPUDashboard/Cards/MetricsCard.tsx
- src/components/GPUDashboard/Cards/InfoCard.tsx
- i18n-scripts/build-i18n.sh
- .stylelintrc.yaml
- deployment/console-plugin-nvidia-gpu/Chart.yaml
- i18n-scripts/common.js
- src/components/shared/OverviewDetailItem.css
- src/components/NotAvailable.tsx
- Dockerfile
- tsconfig.json
- src/components/GPUDashboard/Cards/WorkloadsCard.css
- .dockerignore
- i18n-scripts/lexers.js
🧰 Additional context used
🧬 Code graph analysis (2)
src/components/GPUDashboard/GPUDashboard.tsx (1)
src/components/GPUDashboard/Cards/GPUDashboardGraphs.tsx (1)
GPUDashboardGraphs(48-146)
webpack.config.ts (3)
i18next-parser.config.js (1)
require(2-2)i18n-scripts/common.js (1)
path(2-2)i18n-scripts/set-english-defaults.js (1)
path(2-2)
🔇 Additional comments (32)
src/components/PodStatus/PodStatus.tsx (4)
1-7: Imports updated correctly for PatternFly v6.The migration from
Text/TextContent/TextVariantstoContentaligns with PatternFly v6's component consolidation.
9-81: No changes in podPhase function.
166-174: No changes in PodStatusPopover component.
176-186: No changes in helper functions..gitignore (1)
77-77: LGTM! Cache pattern appropriately extended.The
.cache_*wildcard pattern correctly extends coverage for build tool cache artifacts.src/components/GPUDashboard/Cards/WorkloadsCard.tsx (2)
48-55: CSS class naming convention updated correctly.The
co-break-word→ng-break-wordrename aligns with the PR objective to follow template CSS naming conventions with theng-prefix.
39-46: Pattern is correct for PatternFly v6.The
iconprop onEmptyStateis the correct PatternFly v6 pattern, automatically wrapping the icon component internally. This properly replaces the v4 child component approach.src/components/GPUDashboard/GPUDashboard.tsx (1)
24-57: Layout simplification looks good.The removal of the outer
Pagewrapper and direct use ofPageSectionwithStack-based layout is cleaner. The component hierarchy is well-organized with appropriate PatternFly v6 layout components.src/components/GPUDashboard/GPUSelector.tsx (2)
24-36: Disabled option handling is correct.The
isDisabledprop onSelectOptionfor loading and error states follows PatternFly v6 patterns correctly.
39-59: PatternFly v6 Select implementation is correct.The migration properly uses the v6 API with the toggle renderer function pattern, correctly forwarding the
toggleRefto the MenuToggle component. The props (isOpen,selected,onSelect,onOpenChange,SelectList) all match the current PatternFly v6 API.src/components/shared/OverviewDetailItem.tsx (2)
6-12: Well-designed props interface.The props type is clean and properly typed with appropriate optional fields for loading/error states.
21-28: Loading and error state handling is correct.The priority order (loading → error → children) is sensible. The Spinner with
size="md"is appropriate for inline loading indication.src/components/ClusterOverview/GPUStatus.tsx (2)
115-116: LGTM!The
okColorstring is correctly passed to thecolorprop. The migration fromokColor.value(PatternFly v4 object export) to the direct CSS variable string is consistent across both health status icons.Also applies to: 150-151
18-19: Migration to PatternFly v6 CSS variable is correct.The token
--pf-t--global--color--status--success--defaultis the correct semantic design token for success status colors in PatternFly v6. The CSS custom property approach with fallback is the proper pattern for PF6, and the implementation is sound.README.md (1)
24-24: LGTM!Clear documentation of OpenShift version compatibility, distinguishing between main branch (4.19+) and release branch (4.12-4.18) requirements. This aligns well with the PR's compatibility upgrade objectives.
webpack.config.ts (4)
61-73: Dev server security note:allowedHosts: 'all'is acceptable for local development only.This configuration allows connections from any host, which is appropriate for the development workflow described in the README. The dev server should never be exposed in production environments.
7-8: LGTM!The CommonJS require with eslint-disable is an acceptable workaround for copy-webpack-plugin's module format. The patterns syntax and path resolution are correct.
Also applies to: 74-79
10-10: Clean production/development configuration toggle.The
isProdpattern provides clear separation between build modes with appropriate optimizations: hashed filenames for cache busting, deterministic chunk IDs, and minification in production; readable names and source maps in development.Also applies to: 17-17, 22-23, 80-84
42-58: LGTM!SCSS support and ESM resolution configuration are correctly implemented. The
fullySpecified: falserule addresses common ESM import issues in the webpack ecosystem.i18next-parser.config.js (2)
6-13: Configuration aligns with the i18n workflow.
sort: trueensures reproducible locale files, anduseKeysAsDefaultValue: trueworks in conjunction with theset-english-defaults.jsscript to handle pluralization patterns.
1-2: LGTM!The lexers configuration is comprehensive, covering standard web file types with appropriate lexers. The
CustomJSONLexerfor JSON files enables specialized parsing that extracts translation keys matching the format%key%from locale files, which aligns with the i18n workflow improvements in this PR.i18n-scripts/set-english-defaults.js (2)
27-53: Pluralization logic correctly handles i18next-parser 9.x format.The branching logic properly distinguishes between
{{count}}keys (for parameterized plurals) and simple_one/_othersuffixes. Thepluralizelibrary usage is appropriate for generating English plural forms.
62-75: LGTM!The execution flow correctly filters to process only the English locale and includes appropriate error handling with non-zero exit code on failure, ensuring build pipeline visibility.
locales/en/plugin__console-plugin-nvidia-gpu.json (1)
1-85: LGTM!The locale file is well-organized with:
- Alphabetically sorted keys (matching
sort: trueconfig)- Proper i18next pluralization patterns (
_one/_othersuffixes)- Consistent GPU-centric terminology throughout
- Comprehensive coverage for UI elements (Donut charts, Sparklines, status messages)
package.json (4)
5-8: LGTM!The license declaration and Node.js engine constraint align with the PR objectives for upgrading to Node.js 20.
9-19: LGTM!The updated scripts properly support the TypeScript and tooling upgrades. The
type-checkscript is a good addition for CI validation, and the webpack invocation viats-node/registercorrectly enables TypeScript configuration support.
84-86: LGTM!Declaring
i18nextas a peer dependency is correct for a console plugin, as the host application (OpenShift Console) provides the i18next instance. The version aligns with the devDependency for local development.
61-61: Verify webpack version pinning rationale with the development team.Webpack is pinned to exact version
5.75.0(released Nov 2022) while the current stable is 5.104.1 (Dec 2025). Although the commit message mentions fixing a security vulnerability, pinning to an older version may leave newer vulnerabilities unpatched. Clarify whether newer versions have breaking changes incompatible with the plugin, or if there's a documented reason this specific version was chosen. If possible, upgrade to the latest patched version to ensure comprehensive vulnerability coverage.src/utils/units.ts (4)
179-187: LGTM!The
HumanizeFunctype signature update to acceptstring | numberaligns well with theNumber(v)coercion applied in the wrapper functions. This flexibility is useful when dealing with metrics APIs that may return string representations of numbers.
189-208: LGTM!The
Number(v)coercion is applied consistently across all wrapper functions. Non-finite values (NaN, Infinity) are correctly handled downstream in thehumanize()function which normalizes them to 0.
238-249: LGTM!The implementation correctly handles percentage input (0-100 range), dividing by 100 for
formatPercentagewhich usesstyle: 'percent'. Edge case handling is consistent withhumanizeRatio.
223-236: No type compatibility issues found.The function correctly implements the SDK's
Humanizetype signature, which expects a single parameter. The implementation's numeric coercion and edge case handling are appropriate.
empovit
force-pushed
the
update-for-ocp-4.19-no-chart-changes
branch
from
20e8d07 to
6972873
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @package.json:
- Line 61: Update the webpack dependency in package.json from "webpack":
"5.75.0" to a secure 5.x release (e.g., "webpack": "5.94.0" or use a caret range
like "webpack": "^5.94.0") to ensure the CVE-2023-28154 and CVE-2024-43788 fixes
are included; after changing the "webpack" entry, run your package manager
(npm/yarn/pnpm) to reinstall and update lockfile accordingly and verify the
build passes.
In @src/components/GPUDashboard/GPUSelector.tsx:
- Around line 30-31: The SelectOption created when there are no GPUs (in the
branch that sets items when !gpus?.length || gpusError) currently only sets
value and isDisabled, so it renders empty; update that SelectOption (the element
in items) to include display text as children (e.g., t('No GPU found')) so the
option shows the message while keeping value and isDisabled intact.
🧹 Nitpick comments (4)
src/components/GPUDashboard/GPUSelector.tsx (1)
47-56: Consider displaying the GPU model name instead of UUID in the toggle.The toggle currently shows
selectedGPU?.uuidwhich displays a technical identifier to users. Displaying the model name would be more user-friendly.💡 Suggested improvement
<MenuToggle ref={toggleRef} onClick={() => setOpen(!isOpen)} isExpanded={isOpen} isDisabled={isDisabled} > - {selectedGPU?.uuid || t('Select GPU')} + {selectedGPU?.modelName || t('Select GPU')} </MenuToggle>src/components/ClusterOverview/GPUProviders.tsx (1)
45-45: Consider removing or restoring the commentedvalueClassNameprop.The commented
valueClassName="co-select-to-copy"appears to be leftover code. Either restore it if the copy-to-clipboard styling is needed, or remove the comment to reduce noise.i18n-scripts/set-english-defaults.js (1)
8-60: Consider making file I/O consistently async.The function uses synchronous
fs.readFileSync(line 11) but asynchronousfs.promises.writeFile(line 59). This mixing works but is inconsistent. For better consistency and to avoid blocking the event loop on larger files:♻️ Optional: Use async read
-function updateFile(fileName) { - let file; - try { - const fileContent = fs.readFileSync(fileName, 'utf8'); - file = JSON.parse(fileContent); - } catch (err) { - throw new Error(`Failed to read or parse ${fileName}: ${err.message}`); - } +async function updateFile(fileName) { + let file; + try { + const fileContent = await fs.promises.readFile(fileName, 'utf8'); + file = JSON.parse(fileContent); + } catch (err) { + throw new Error(`Failed to read or parse ${fileName}: ${err.message}`); + }package.json (1)
47-47:prettier-stylelintis deprecated and unmaintained.This package hasn't been updated since 2017 and is no longer maintained. Consider removing it and running Prettier and Stylelint separately, which the
lintscript already does.Suggested fix
- "prettier-stylelint": "^0.4.2",
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (33)
.dockerignore.eslintrc.eslintrc.yml.gitignore.prettierrc.yml.stylelintrc.yamlDockerfileREADME.mddeployment/console-plugin-nvidia-gpu/Chart.yamli18n-scripts/build-i18n.shi18n-scripts/common.jsi18n-scripts/lexers.jsi18n-scripts/set-english-defaults.jsi18next-parser.config.jslocales/en/plugin__console-plugin-nvidia-gpu.jsonpackage.jsonsrc/components/ClusterOverview/GPUProviders.tsxsrc/components/ClusterOverview/GPUStatus.tsxsrc/components/GPUDashboard/Cards/InfoCard.tsxsrc/components/GPUDashboard/Cards/MetricsCard.csssrc/components/GPUDashboard/Cards/MetricsCard.tsxsrc/components/GPUDashboard/Cards/WorkloadsCard.csssrc/components/GPUDashboard/Cards/WorkloadsCard.tsxsrc/components/GPUDashboard/GPUDashboard.tsxsrc/components/GPUDashboard/GPUSelector.tsxsrc/components/NotAvailable.csssrc/components/NotAvailable.tsxsrc/components/PodStatus/PodStatus.tsxsrc/components/shared/OverviewDetailItem.csssrc/components/shared/OverviewDetailItem.tsxsrc/utils/units.tstsconfig.jsonwebpack.config.ts
💤 Files with no reviewable changes (2)
- .prettierrc.yml
- .eslintrc
✅ Files skipped from review due to trivial changes (1)
- i18n-scripts/common.js
🚧 Files skipped from review as they are similar to previous changes (18)
- src/components/PodStatus/PodStatus.tsx
- i18n-scripts/lexers.js
- src/components/GPUDashboard/Cards/MetricsCard.tsx
- .stylelintrc.yaml
- .gitignore
- src/components/NotAvailable.tsx
- Dockerfile
- src/components/GPUDashboard/GPUDashboard.tsx
- src/components/GPUDashboard/Cards/InfoCard.tsx
- src/utils/units.ts
- .eslintrc.yml
- src/components/GPUDashboard/Cards/WorkloadsCard.css
- i18n-scripts/build-i18n.sh
- src/components/NotAvailable.css
- src/components/ClusterOverview/GPUStatus.tsx
- webpack.config.ts
- i18next-parser.config.js
- .dockerignore
🧰 Additional context used
🧬 Code graph analysis (1)
i18n-scripts/set-english-defaults.js (3)
i18n-scripts/common.js (2)
fs(1-1)path(2-2)i18next-parser.config.js (1)
require(2-2)i18n-scripts/lexers.js (1)
keys(9-9)
🔇 Additional comments (22)
src/components/GPUDashboard/Cards/MetricsCard.css (1)
8-11: LGTM!The new
.ng-metrics-card__titleclass follows theng-prefix convention established in this PR and correctly replaces the PatternFly utility class for center alignment.src/components/GPUDashboard/GPUSelector.tsx (1)
39-59: LGTM!The PatternFly v6 Select implementation correctly uses the controlled pattern with
isOpen,selected,onSelect, andonOpenChangeprops. TheMenuToggleintegration andSelectListwrapper follow the updated API properly.src/components/shared/OverviewDetailItem.tsx (1)
14-36: LGTM!Clean implementation of a shared detail item component. The loading/error/content state handling is correct, and the
dl/dt/ddstructure is semantically appropriate for term-definition pairs. The component follows theng-prefix convention for CSS classes.src/components/ClusterOverview/GPUProviders.tsx (1)
2-2: LGTM!The import path correctly references the new local
OverviewDetailItemcomponent, aligning with the PR's goal of removing the external@openshift-console/plugin-shareddependency.src/components/shared/OverviewDetailItem.css (1)
1-22: LGTM!Well-structured CSS following the
ng-prefix convention. The PatternFly v6 design tokens (--pf-t--global--*) are correctly used for consistent theming, and the word-break properties ensure proper handling of long values.src/components/GPUDashboard/Cards/WorkloadsCard.tsx (2)
48-55: LGTM!CSS class rename from
co-break-wordtong-break-wordaligns with the template CSS naming conventions (ng- prefix) stated in the PR objectives. The utility class is correctly defined in the companion CSS file.
39-46: The EmptyState implementation is correct. PatternFly v6 accepts theiconprop with React components likeCubesIcondirectly, which matches your implementation. No changes needed.README.md (1)
24-24: Documentation update looks good.The prerequisites correctly reflect that the main branch now requires OpenShift 4.19+, while the release branch supports 4.12-4.18. This aligns with the Chart.yaml version bump to 0.3.0.
tsconfig.json (2)
10-12: Good TypeScript strictness improvements.Enabling
strict: trueis the right approach as it subsumesnoImplicitAnyandstrictNullCheckswhile adding additional checks. AddingskipLibCheck: trueimproves build performance without sacrificing type safety in your own code.
16-22: ts-node configuration supports the build tooling.The
ts-nodeblock withmodule: "commonjs"allows TypeScript-based configuration files (likewebpack.config.ts) and i18n scripts to run correctly in Node.js. SettingtranspileOnly: truespeeds up execution by skipping type checking during script runs.deployment/console-plugin-nvidia-gpu/Chart.yaml (2)
4-4: Version requirement update is accurate.The description correctly states the new 4.19+ requirement. Consider whether users on 4.12-4.18 should be directed to use an older chart version (e.g., 0.2.x) — this could be clarified in chart notes or the README's deployment section.
15-15: Version bump to 0.3.0 is appropriate.The minor version bump correctly signals backward-incompatible changes (new OCP version requirement). This aligns with the PR objectives.
locales/en/plugin__console-plugin-nvidia-gpu.json (2)
2-5: Pluralization format follows i18next conventions.The
_one/_othersuffix pattern with{{count}}interpolation is the correct i18next-parser 9.x format. The singular/plural values are properly defined.
1-85: Locale file structure looks good.The keys are alphabetically sorted and cover the GPU dashboard terminology comprehensively. The file is well-organized with consistent naming patterns (Donut/Sparkline prefixes for chart labels, GPU-centric vocabulary).
i18n-scripts/set-english-defaults.js (3)
32-53: Pluralization logic correctly handles i18next-parser 9.x conventions.The implementation properly distinguishes between:
- Keys with
{{count}}and_onesuffix → singular form- Keys with
{{count}}and_othersuffix → plural form- Simple
_one/_othersuffixes → base key or pluralized baseThis aligns with the locale file structure in
plugin__console-plugin-nvidia-gpu.json.
62-66: Only English locale processing is intentional.The filter for
'en'locale is appropriate since this script sets English defaults — other locales would need translation rather than auto-generated values.
68-75: IIFE with proper error handling.The async IIFE correctly handles errors by logging and exiting with a non-zero status code, which is appropriate for build scripts.
package.json (5)
5-8: LGTM!Adding the license field and Node.js engine constraint is good practice. The
>=20.0.0requirement aligns with the Dockerfile update mentioned in the PR objectives.
11-19: LGTM!The script updates are well-structured:
- Build scripts properly set
NODE_ENVfor production builds- The
type-checkscript supports the TypeScript strict mode objective- The webpack wrapper using
ts-node/registercorrectly handles the TypeScript webpack config
28-40: LGTM!The TypeScript tooling versions are internally consistent.
@types/nodecorrectly matches the Node.js 20 engine requirement.
84-86: LGTM!Declaring
i18nextas a peer dependency is correct for a plugin, ensuring the host console provides the i18n runtime. The version matches the devDependency for local development consistency.
24-27: No changes required. PatternFly v6.2.2 explicitly supports React 17, which is the version currently installed (^17.0.1). The dependencies are compatible.Likely an incorrect or invalid review comment.
empovit
force-pushed
the
update-for-ocp-4.19-no-chart-changes
branch
from
6972873 to
cd77fe9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
.dockerignore (1)
1-4: LGTM! Essential Docker ignore patterns are in place.The file correctly excludes the most critical directories and files for a Node.js project. These patterns will optimize Docker build context size and prevent unnecessary files from being copied.
💡 Optional: Consider additional patterns for a more comprehensive exclusion list
While the current patterns are sufficient, you may optionally add:
node_modules dist .git *.log +.gitignore +README.md +*.md +test +tests +**/*.test.js +**/*.test.ts +**/*.spec.js +**/*.spec.ts +.vscode +.idea +.DS_Store +.env* +coverageThese additional patterns can further reduce build context size and prevent development-only files from being included.
src/components/GPUDashboard/GPUSelector.tsx (1)
46-46: Variable shadowing reduces clarity.The parameter
isOpenshadows the state variable on line 18. Consider renaming for readability.✨ Suggested fix
- onOpenChange={(isOpen) => setOpen(isOpen)} + onOpenChange={(open) => setOpen(open)}i18n-scripts/lexers.js (1)
24-27: Nit: Redundant reassignment in catch block.The
keys = []assignment at line 26 is redundant sincekeyswas already initialized as an empty array at line 9 and won't have been modified if parsing fails at thejsonc.parsecall.♻️ Optional simplification
} catch (e) { console.error('Failed to parse as JSON.', filename, e); - keys = []; }i18n-scripts/set-english-defaults.js (1)
8-60: Consider using consistent async I/O throughout.The function mixes synchronous
fs.readFileSync(line 11) with asynchronousfs.promises.writeFile(line 59). While functional, this inconsistency can be confusing and blocks the event loop during reads.♻️ Suggested refactor for consistent async I/O
-function updateFile(fileName) { - let file; - try { - const fileContent = fs.readFileSync(fileName, 'utf8'); - file = JSON.parse(fileContent); - } catch (err) { - throw new Error(`Failed to read or parse ${fileName}: ${err.message}`); - } +async function updateFile(fileName) { + let file; + try { + const fileContent = await fs.promises.readFile(fileName, 'utf8'); + file = JSON.parse(fileContent); + } catch (err) { + throw new Error(`Failed to read or parse ${fileName}: ${err.message}`); + }webpack.config.ts (1)
61-73: Note: Permissive dev server settings.
allowedHosts: 'all'and the wildcard CORS headers are fine for local development but could pose a risk if the dev server is accidentally exposed on a shared network. Consider documenting that this configuration is for local development only.src/utils/units.ts (1)
209-221: Minor inconsistency in rounding behavior.Two observations:
Zero displays as "0m": When
numValueis 0, the conditionnumValue < 1is true, sounit = 'm'. This results in "0m" rather than "0".Inconsistent rounding of returned
value: FornumValue < 1, the returnedvalueis rounded (line 214), but fornumValue >= 1, it's returned as-is. The display is fine (handled byformatValue), but consumers relying on thevalueproperty see inconsistent precision.If this matches the previous behavior, no action needed; otherwise consider:
Optional fix for consistency
export const humanizeCpuCores: HumanizeFunc = (v) => { let numValue = Number(v); if (!isFinite(numValue) || numValue === 0) { numValue = 0; } - const value = numValue < 1 ? round(numValue * 1000) : numValue; + const value = numValue < 1 ? round(numValue * 1000) : round(numValue); const unit = numValue < 1 ? 'm' : '';
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (33)
.dockerignore.eslintrc.eslintrc.yml.gitignore.prettierrc.yml.stylelintrc.yamlDockerfileREADME.mddeployment/console-plugin-nvidia-gpu/Chart.yamli18n-scripts/build-i18n.shi18n-scripts/common.jsi18n-scripts/lexers.jsi18n-scripts/set-english-defaults.jsi18next-parser.config.jslocales/en/plugin__console-plugin-nvidia-gpu.jsonpackage.jsonsrc/components/ClusterOverview/GPUProviders.tsxsrc/components/ClusterOverview/GPUStatus.tsxsrc/components/GPUDashboard/Cards/InfoCard.tsxsrc/components/GPUDashboard/Cards/MetricsCard.csssrc/components/GPUDashboard/Cards/MetricsCard.tsxsrc/components/GPUDashboard/Cards/WorkloadsCard.csssrc/components/GPUDashboard/Cards/WorkloadsCard.tsxsrc/components/GPUDashboard/GPUDashboard.tsxsrc/components/GPUDashboard/GPUSelector.tsxsrc/components/NotAvailable.csssrc/components/NotAvailable.tsxsrc/components/PodStatus/PodStatus.tsxsrc/components/shared/OverviewDetailItem.csssrc/components/shared/OverviewDetailItem.tsxsrc/utils/units.tstsconfig.jsonwebpack.config.ts
💤 Files with no reviewable changes (2)
- .prettierrc.yml
- .eslintrc
🚧 Files skipped from review as they are similar to previous changes (15)
- src/components/NotAvailable.css
- src/components/NotAvailable.tsx
- i18n-scripts/common.js
- src/components/PodStatus/PodStatus.tsx
- Dockerfile
- .gitignore
- README.md
- deployment/console-plugin-nvidia-gpu/Chart.yaml
- src/components/shared/OverviewDetailItem.tsx
- tsconfig.json
- src/components/GPUDashboard/Cards/WorkloadsCard.css
- src/components/GPUDashboard/GPUDashboard.tsx
- package.json
- src/components/ClusterOverview/GPUStatus.tsx
- src/components/shared/OverviewDetailItem.css
🧰 Additional context used
🧬 Code graph analysis (1)
i18n-scripts/lexers.js (2)
i18next-parser.config.js (1)
require(2-2)i18n-scripts/set-english-defaults.js (2)
keys(22-22)key(25-25)
🔇 Additional comments (29)
src/components/GPUDashboard/Cards/WorkloadsCard.tsx (2)
39-46: LGTM! Correct PatternFly v6 migration for EmptyState.The
iconprop onEmptyStateis the correct API for PatternFly v6, replacing the deprecatedEmptyStateIconcomponent pattern from v4.
48-55: LGTM! CSS class naming follows plugin conventions.Renaming from
co-break-word(Console internal) tong-break-word(plugin namespace) correctly decouples the plugin from OpenShift Console's internal CSS classes, aligning with the PR's CSS naming convention objectives.src/components/GPUDashboard/Cards/MetricsCard.css (1)
8-11: LGTM!The new
.ng-metrics-card__titleclass correctly replaces the PatternFly utility classpf-u-text-align-center, following theng-prefix convention and maintaining consistent BEM-like naming with sibling selectors in this file.src/components/GPUDashboard/Cards/MetricsCard.tsx (1)
95-95: LGTM!The class name change from
pf-u-text-align-centertong-metrics-card__titlecorrectly aligns with the PatternFly v6 migration and the project'sng-prefix naming convention. The corresponding CSS definition is properly imported from./MetricsCard.css.src/components/ClusterOverview/GPUProviders.tsx (1)
2-2: Verify import change and local component implementation.The import path was changed from
@openshift-console/plugin-sharedto../shared/OverviewDetailItem. Confirm that:
- The local
OverviewDetailItemcomponent exists atsrc/components/shared/OverviewDetailItem.tsx- It exports the same interface (accepting
title,isLoading,error,childrenprops)- All usages across the codebase are compatible with this change
src/components/GPUDashboard/GPUSelector.tsx (4)
2-10: LGTM on PatternFly v6 migration.The imports correctly reflect the PF v6 Select pattern using
SelectList,MenuToggle, andMenuToggleElement. This aligns with the PR objective of upgrading to PatternFly v6.
14-20: Disabled state logic is correct.The
isDisabledboolean properly accounts for loading, error, and empty GPU states.
22-36: LGTM on items generation.The type annotation
React.ReactElement<SelectOptionProps>[]is appropriately specific, and theisDisabledprop usage aligns with PF v6 patterns.
54-54: Consider displaying a user-friendly GPU identifier.The toggle shows the raw UUID, which may be long and not user-friendly. If
modelNameis available on the GPU object (as suggested by its use elsewhere), consider displaying it instead—or a combination if multiple GPUs of the same model might exist..eslintrc.yml (1)
1-25: LGTM!The ESLint configuration is well-structured with appropriate extends for TypeScript and React projects. Key highlights:
- Prettier integration as error ensures consistent formatting
react/prop-types: offis correct since TypeScript handles prop validation- React version detection avoids manual version maintenance
i18n-scripts/build-i18n.sh (1)
1-7: LGTM!The script follows shell best practices:
set -exuo pipefailensures proper error handling and debugging output- File pattern correctly covers all relevant source types including JSON for the custom lexer
- Output path template properly escapes
$LOCALEand$NAMESPACEfor i18next interpretationi18n-scripts/lexers.js (1)
7-29: LGTM!The custom lexer implementation is correct:
- Extends
EventEmitteras required by i18next-parser for custom lexers- Uses
comment-jsonto support JSON files with comments- Gracefully handles parse errors without breaking the build
.stylelintrc.yaml (1)
1-26: LGTM!Excellent Stylelint configuration for a console plugin:
- Dark mode support enforced by disallowing named/hex colors and
rgb()in favor of PatternFly CSS variables- Reserved prefixes (
pf-,co-) properly blocked to avoid conflicts with PatternFly and OpenShift console styles- Element selector restrictions with appropriate exceptions prevent unintended global style impacts
- Clear inline comments document the rationale for each rule
i18next-parser.config.js (1)
1-31: LGTM!The i18next-parser configuration is correctly set up:
CustomJSONLexerproperly imported and registered for JSON file parsing- Built-in lexers correctly referenced by string names while custom lexer is passed as class reference (this is the expected pattern for i18next-parser)
sort: trueensures consistent ordering in locale files- Comprehensive lexer mappings cover all relevant file types in the project
i18n-scripts/set-english-defaults.js (3)
1-6: LGTM on imports and constants.Clean setup with necessary dependencies for file I/O and pluralization handling.
62-66: LGTM!The filter logic correctly targets only the English locale folder for processing.
68-75: LGTM!Proper error handling with informative logging and appropriate exit code for build script integration.
webpack.config.ts (5)
7-10: LGTM on the environment-based configuration setup.The CommonJS require for CopyWebpackPlugin with the eslint-disable comment is a reasonable workaround for TypeScript/ESM interop. The
isProdflag provides clean conditional logic throughout.
16-24: Well-structured output configuration.Hash-based filenames in production enable effective cache busting, while development retains readable names for debugging. Good practice.
42-58: Good additions for SCSS and ESM compatibility.The loader chain order is correct for SCSS processing, and
fullySpecified: falseaddresses common ESM interop issues with packages that omit file extensions.
74-79: LGTM!The CopyWebpackPlugin configuration correctly resolves the locales directory and copies it to the output. This integrates well with the i18n tooling pipeline.
80-84: Solid optimization settings.Disabling source maps in production, using deterministic chunk IDs for caching, and conditional minimization are all best practices for production builds.
locales/en/plugin__console-plugin-nvidia-gpu.json (3)
2-5: LGTM on pluralization keys.The
_oneand_othersuffixes follow i18next-parser 9.x conventions correctly.
14-21: Well-organized chart component keys.The
DonutandSparklineprefixed keys provide clear categorization for different visualization components. The naming convention is consistent throughout.Also applies to: 70-77
1-85: LGTM!The locale file is well-structured with comprehensive GPU-related vocabulary. Keys are alphabetically sorted and follow i18next conventions. The file integrates properly with the new i18n tooling pipeline.
src/components/GPUDashboard/Cards/InfoCard.tsx (1)
3-3: Verify the localOverviewDetailItemcomponent implementation.The import path change from
@openshift-console/plugin-sharedto the local../../shared/OverviewDetailItemis consistent with the SDK upgrade pattern. However, the local component implementation atsrc/components/shared/OverviewDetailItem.tsxneeds manual verification to ensure it properly exports the same interface and behavior as the original library component, given that@openshift-console/plugin-sharedno longer exports it in SDK 1.4.0.src/utils/units.ts (3)
179-187: LGTM – Type widening tostring | numberis appropriate.This change provides flexibility for callers passing string values (common with Prometheus metric results) while the internal
Number()coercion ensures type safety downstream.
189-208: LGTM – Consistent numeric coercion across all wrappers.The
Number(v)coercion is uniformly applied, and downstreamhumanize()already handles non-finite values at lines 147-149.
223-249: Logic is correct; verify SDKHumanizetype compatibility.The numeric coercion and normalization logic is sound. The distinction between
humanizeRatio(input: 0-1) andhumanizePercentage(input: 0-100) is correctly preserved.However, these functions now use the SDK's
Humanizetype instead of the localHumanizeFunc. Since the SDK upgraded from 0.0.11 to 1.4.0, please verify that theHumanizetype signature is compatible with the single-parameter functions here.OpenShift dynamic-plugin-sdk Humanize type signature 1.4.0
empovit
force-pushed
the
update-for-ocp-4.19-no-chart-changes
branch
from
cd77fe9 to
c3712f1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@package.json`:
- Around line 86-88: package.json currently lists i18next in both "dependencies"
and "peerDependencies", which is redundant and can cause duplicate instances;
decide whether the host should provide i18next or the package should bundle it:
if the host supplies it for module federation, remove i18next from
"dependencies" and keep it in "peerDependencies"; if this package should bundle
i18next, remove it from "peerDependencies" and keep it in "dependencies"; also
evaluate whether react-i18next should be moved to "peerDependencies" (or added
there alongside i18next) to enforce a single shared instance in host
applications and avoid version conflicts.
♻️ Duplicate comments (1)
package.json (1)
63-63: Acknowledged: webpack 5.75.0 pinned due to SDK compatibility.Per past discussions, this version is required by
@openshift-console/[email protected]. The security vulnerabilities (CVE-2023-28154, CVE-2024-43788) remain a concern but cannot be addressed until the SDK is updated.
🧹 Nitpick comments (6)
i18n-scripts/common.js (1)
14-27: Simplify async function wrapper.The IIFE wrapper around the async function is unnecessary. A direct async function is cleaner and equivalent.
♻️ Suggested simplification
- parseFolder(directory, argFunction, packageDir) { - return (async () => { - try { - const files = await fs.promises.readdir(directory); - for (const file of files) { - const filePath = path.join(directory, file); - await argFunction(filePath, packageDir); - } - } catch (e) { - console.error(`Failed to parseFolder ${directory}:`, e); - throw e; - } - })(); - }, + async parseFolder(directory, argFunction, packageDir) { + try { + const files = await fs.promises.readdir(directory); + for (const file of files) { + const filePath = path.join(directory, file); + await argFunction(filePath, packageDir); + } + } catch (e) { + console.error(`Failed to parseFolder ${directory}:`, e); + throw e; + } + },i18n-scripts/set-english-defaults.js (1)
59-60: Consider addingasynckeyword for clarity.The function returns a Promise but isn't marked
async. While functional, marking itasyncwould make the intent clearer and enableawaitusage within the function if needed later.♻️ Suggested change
-function updateFile(fileName) { +async function updateFile(fileName) { // ... existing code ... - return fs.promises.writeFile(fileName, JSON.stringify(updatedFile, null, 2)); + await fs.promises.writeFile(fileName, JSON.stringify(updatedFile, null, 2)); }package.json (2)
62-64: TypeScript 4.9.5 is outdated but acceptable given ecosystem constraints.TypeScript 5.x offers improved type inference and performance, but staying on 4.9.5 maintains compatibility with the current tooling stack. Consider upgrading when SDK constraints allow.
35-36: Consider upgrading TypeScript ESLint packages.
@typescript-eslint/eslint-pluginand@typescript-eslint/parserat^5.14.0are significantly outdated. The current stable is v8.42.0, which offers improved TypeScript support. Upgrading requires ESLint ^8.57.0 or higher (not 9.x specifically)—you can stay on the 8.x line if needed by bumping ESLint from ^8.10.0 to ^8.57.0+.locales/en/plugin__console-plugin-nvidia-gpu.json (2)
14-21: Consider whether "Donut" prefix adds value for translators.Keys like "Donut GPU utilization" and "Donut Encoder utilization" appear to be chart-type identifiers. If these are purely for accessibility labels (aria-labels), they may confuse translators. Consider adding translator comments or using a more descriptive pattern.
70-77: Same consideration for "Sparkline" prefixed keys.Similar to the "Donut" keys, "Sparkline" prefixed entries could benefit from translator context. These appear to be accessibility labels for chart components.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (33)
.dockerignore.eslintrc.eslintrc.yml.gitignore.prettierrc.yml.stylelintrc.yamlDockerfileREADME.mddeployment/console-plugin-nvidia-gpu/Chart.yamli18n-scripts/build-i18n.shi18n-scripts/common.jsi18n-scripts/lexers.jsi18n-scripts/set-english-defaults.jsi18next-parser.config.jslocales/en/plugin__console-plugin-nvidia-gpu.jsonpackage.jsonsrc/components/ClusterOverview/GPUProviders.tsxsrc/components/ClusterOverview/GPUStatus.tsxsrc/components/GPUDashboard/Cards/InfoCard.tsxsrc/components/GPUDashboard/Cards/MetricsCard.csssrc/components/GPUDashboard/Cards/MetricsCard.tsxsrc/components/GPUDashboard/Cards/WorkloadsCard.csssrc/components/GPUDashboard/Cards/WorkloadsCard.tsxsrc/components/GPUDashboard/GPUDashboard.tsxsrc/components/GPUDashboard/GPUSelector.tsxsrc/components/NotAvailable.csssrc/components/NotAvailable.tsxsrc/components/PodStatus/PodStatus.tsxsrc/components/shared/OverviewDetailItem.csssrc/components/shared/OverviewDetailItem.tsxsrc/utils/units.tstsconfig.jsonwebpack.config.ts
💤 Files with no reviewable changes (2)
- .prettierrc.yml
- .eslintrc
🚧 Files skipped from review as they are similar to previous changes (12)
- src/components/GPUDashboard/GPUDashboard.tsx
- src/components/NotAvailable.css
- src/components/GPUDashboard/Cards/MetricsCard.css
- .stylelintrc.yaml
- src/components/shared/OverviewDetailItem.css
- src/components/shared/OverviewDetailItem.tsx
- i18n-scripts/build-i18n.sh
- .eslintrc.yml
- src/components/GPUDashboard/Cards/InfoCard.tsx
- src/components/GPUDashboard/Cards/MetricsCard.tsx
- deployment/console-plugin-nvidia-gpu/Chart.yaml
- .dockerignore
🧰 Additional context used
🧬 Code graph analysis (4)
i18n-scripts/common.js (2)
i18n-scripts/set-english-defaults.js (3)
fs(1-1)path(2-2)file(9-9)i18next-parser.config.js (1)
require(2-2)
i18n-scripts/set-english-defaults.js (3)
i18n-scripts/common.js (2)
fs(1-1)path(2-2)i18next-parser.config.js (1)
require(2-2)i18n-scripts/lexers.js (1)
keys(9-9)
src/components/NotAvailable.tsx (1)
src/i18n.ts (1)
useTranslation(15-17)
i18n-scripts/lexers.js (2)
i18next-parser.config.js (1)
require(2-2)i18n-scripts/set-english-defaults.js (2)
keys(22-22)key(25-25)
🔇 Additional comments (40)
src/components/NotAvailable.tsx (1)
4-8: LGTM!The migration from PatternFly utility class (
pf-u-color-200) to a local component-scoped CSS class (ng-not-available) aligns well with the PR's stated objectives for the PatternFly v6 upgrade and following theng-prefix naming convention. The functional behavior is preserved.src/components/ClusterOverview/GPUStatus.tsx (2)
18-19: LGTM!The CSS variable approach with the correct PatternFly v6 fallback (
#3e8635) is a clean solution for the migration. This provides proper theming support while ensuring graceful degradation.
115-116: Consistent usage of the new color constant.Both health check functions correctly apply the
okColorconstant for success states, maintaining visual consistency across the component.Also applies to: 150-151
src/components/PodStatus/PodStatus.tsx (2)
3-3: LGTM — PatternFly v6 import migration.The
Contentimport correctly replaces the deprecatedText/TextContent/TextVariantscomponents from PatternFly v4.
116-149: LGTM — PatternFly v6 migration and URL path fix.The migration from
Text/TextContenttoContentwithcomponent="p"is correct for PatternFly v6. The fragment wrapper is appropriate here. Additionally, the URL paths now correctly referencepod.metadata.name(fixing what appears to have been a typo).src/components/ClusterOverview/GPUProviders.tsx (1)
2-2: LGTM — Import updated to use local component.This change connects GPUProviders.tsx to the locally managed OverviewDetailItem implementation, which is appropriate given the Console SDK upgrade from 0.0.11 to 1.4.0. The component interface perfectly matches the props being passed:
title,isLoading,error, andchildren. ThevalueClassNameprop is available in the local component but is simply commented out in this specific usage.src/utils/units.ts (5)
179-187: LGTM!The type broadening to accept
string | numberimproves flexibility for handling API responses that may return numeric values as strings, while maintaining a consistentnumberreturn type.
189-208: LGTM!Consistent
Number(v)coercion across all wrapper functions aligns with the updated type signature. Non-finite values (includingNaNfrom invalid strings) are properly normalized to0by the centralhumanize()function.
209-221: LGTM!The numeric coercion and normalization logic correctly handles edge cases. The "0m" output for zero values is retained per previous discussion.
223-236: LGTM!Proper numeric coercion and normalization. Using the SDK's
Humanizetype improves consistency with the dynamic plugin ecosystem. The fraction digits calculation correctly scales the ratio for percentage-appropriate precision.
238-249: LGTM!Consistent numeric handling with appropriate scaling (dividing by 100) since this function expects percentage-scale input (0-100) rather than ratio-scale (0-1). The SDK
Humanizetype alignment is a good consistency improvement.src/components/GPUDashboard/Cards/WorkloadsCard.css (1)
6-9: LGTM!The new
.ng-break-wordclass correctly implements word-breaking behavior for table cells. Using bothword-break: break-wordandoverflow-wrap: anywhereensures robust text wrapping across browsers for long pod names and namespaces. Theng-prefix aligns with the template CSS naming conventions.src/components/GPUDashboard/Cards/WorkloadsCard.tsx (2)
48-55: LGTM!The className migration from
co-break-wordto the locally-definedng-break-wordcorrectly removes the dependency on OpenShift Console global styles. This aligns with the PR objective to follow template CSS naming conventions and makes the component self-contained.
39-46: EmptyStateiconprop usage is correct for PatternFly v6.The migration from the PF v4
EmptyStateIconwrapper to the v6iconprop is implemented correctly. The prop accepts a React component reference (likeCubesIcon), which is then automatically wrapped by the internalEmptyStateIconcomponent..gitignore (1)
75-77: LGTM!The addition of
.cache_*appropriately extends the existing.cacheignore pattern to cover cache directories with suffixes, which aligns with the build tooling updates in this PR.tsconfig.json (1)
7-22: LGTM!The TypeScript configuration updates are well-aligned with the PR objectives:
strict: trueenables comprehensive type checking.sourceMap: trueimproves debugging experience.- The
ts-nodeblock withtranspileOnly: trueandmodule: commonjsenables efficient execution of TypeScript scripts (e.g., the i18n tooling) without full type-checking overhead.README.md (1)
24-24: LGTM!The prerequisite update correctly documents the OpenShift 4.19+ requirement for the main branch while preserving backward compatibility information for the release branch.
i18next-parser.config.js (1)
1-31: LGTM!The i18next-parser configuration is well-structured:
sort: trueensures deterministic, reproducible locale catalogs.- The
lexersmapping comprehensively covers the project's file types.CustomJSONLexeris correctly passed as a class reference for custom lexer handling.The eslint-disable comments are appropriate for this CommonJS configuration file.
i18n-scripts/lexers.js (1)
7-29: LGTM!The
CustomJSONLexercorrectly implements the i18next-parser lexer interface. The regex pattern/^%(.+)%$/appropriately captures localization keys in the%key%format, and the error handling gracefully degrades by returning an empty array on parse failures. Thecomment-jsondependency is properly declared inpackage.json.Dockerfile (2)
1-7: LGTM! Build stage properly upgraded to Node.js 20.The upgrade from
ubi8/nodejs-16toubi9/nodejs-20aligns with the PR objectives. The conditional yarn installation (command -v yarn || npm i -g yarn) is a good defensive pattern.
9-13: LGTM! Runtime stage correctly configured.The final stage properly references the renamed build stage (
--from=build), andUSER 1001ensures the container runs as a non-root user for security.src/components/GPUDashboard/GPUSelector.tsx (3)
2-10: LGTM! PatternFly v6 Select imports correctly updated.The imports align with PF v6's Select component API, including
MenuTogglefor the custom toggle renderer andSelectListfor the options container.
24-41: LGTM! Select options properly structured with display text.The loading and "No GPU found" options now correctly include children text for display, and the GPU options use
uuidas both value and display with model name as description.
45-65: PatternFly v6 Select API usage is correct.The controlled Select implementation properly uses
isOpen,selected,onSelect, andonOpenChangeprops with correct signatures. TheMenuToggletoggle renderer follows the expected pattern.i18n-scripts/common.js (2)
4-13: LGTM! Defensive error handling for directory check.The
isDirectoryhelper properly catches errors fromlstatSync(e.g., path doesn't exist) and returnsfalse, which is appropriate for a predicate function.
28-35: LGTM! Sync delete with proper error handling.The
deleteFilehelper logs the error before rethrowing, which aids debugging.webpack.config.ts (4)
7-10: LGTM! Production/development configuration flag.Using
isProdto conditionally configure webpack is a clean pattern. The CommonJS require forcopy-webpack-pluginis acceptable given the eslint-disable comment.
42-58: LGTM! SCSS and ESM support added.Adding
sass-loaderenables SCSS compilation for the new styling changes. ThefullySpecified: falserule for.mjs/.jsfiles is necessary for proper ESM module resolution in webpack 5.
61-73: Note:allowedHosts: 'all'is permissive.This setting allows connections from any host to the dev server, which is convenient for development but bypasses host header checks. This is acceptable for local development but ensure this configuration isn't used in any publicly accessible environment.
74-84: LGTM! Production optimizations properly configured.The
deterministicchunk IDs and minification in production, combined withnamedchunks and source maps in development, provide optimal builds for each environment.i18n-scripts/set-english-defaults.js (3)
8-19: LGTM! JSON file reading with proper validation.The implementation correctly uses
fs.readFileSync(addressing the previous caching concern) and validates the parsed content is a non-array object before processing.
24-57: LGTM! Pluralization transformation logic is comprehensive.The key transformation handles all i18next-parser 9.x pluralization cases:
{{count}} ..._one→ singular form{{count}} ..._other→ plural form- Simple
_one/_othersuffixes- Default fallback using key as value
68-75: LGTM! Entry point with proper error handling.The IIFE properly catches errors and exits with a non-zero status code, which is appropriate for a build script that should fail the pipeline on error.
package.json (5)
5-8: LGTM! License and engine requirements properly specified.Adding the Apache-2.0 license and requiring Node.js >=20.0.0 aligns with the PR objectives for upgrading to Node.js 20.
11-19: LGTM! Scripts modernized appropriately.The build scripts now use a cleaner webpack invocation pattern, and the i18n pipeline is properly wired to the new extraction scripts. The addition of
type-checkscript supports the TypeScript strict mode enablement mentioned in PR objectives.
22-24: Good documentation of version constraints.The JSON comments clearly explain why webpack and react-charts are pinned, addressing the SDK compatibility constraints. This helps future maintainers understand the dependencies.
25-30: PatternFly v6 packages aligned, with documented exception for react-charts.The SDK and PatternFly core/icons/table packages are updated to v6 as intended. The react-charts v7 pinning is documented and explained in past discussions (module federation incompatibility with v8).
50-54: React 17 with React Router 5 is appropriate for SDK compatibility.The pinned React 17 and React Router 5.3.x versions align with the OpenShift Console SDK requirements for module federation compatibility.
locales/en/plugin__console-plugin-nvidia-gpu.json (2)
2-5: LGTM! Pluralization values correctly implemented.The plural keys now have proper display values without the suffix repetition. The
_onevariants use singular forms and_othervariants use plural forms as expected by i18next.
1-85: LGTM! GPU-centric vocabulary is comprehensive and well-structured.The locale file covers the expected UI elements for GPU monitoring: metrics, status indicators, actions, and vendor names. Keys are alphabetically sorted and follow consistent naming conventions.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
empovit
force-pushed
the
update-for-ocp-4.19-no-chart-changes
branch
from
c3712f1 to
e518398
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
empovit
changed the title
|
This was tested on 4.19 (HCP cluster with three workers, one GPU each) and 4.20 (single node, single GPU). |
- Update Console SDK 0.0.11 → 1.4.0 - Upgrade PatternFly v4 → v6 - Upgrade to Node.js 20 - Enable TypeScript strict mode - Update CSS and UI controls - Bump Helm chart version to 0.3.0 (backward compatible) - Update dependencies
empovit
force-pushed
the
update-for-ocp-4.19-no-chart-changes
branch
from
e518398 to
831f050
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/PodStatus/PodStatus.tsx (1)
64-67: Potential null reference in existing logic.If
waitingis truthy and!waiting.reasonis true, butterminatedis undefined (which is possible since theifchain only checkswaiting), accessingterminated.signalon line 65 will throw a TypeError.This appears to be pre-existing code, but worth flagging during this review since it could cause runtime crashes.
🐛 Proposed fix
} else if (waiting && !waiting.reason) { - phase = terminated.signal + phase = terminated?.signal ? `Signal:${terminated.signal}` - : `ExitCode:${terminated.exitCode}`; + : `ExitCode:${terminated?.exitCode}`;
♻️ Duplicate comments (1)
package.json (1)
59-59: Acknowledged: webpack 5.75.0 pinned for SDK compatibility.As noted in prior discussion, webpack cannot be upgraded due to compatibility constraints with
@openshift-console/[email protected]. However, this version has known CVEs (CVE-2023-28154, CVE-2024-43788). Consider documenting this constraint and the associated risk in aDEPENDENCIES.mdor similar file for future maintainers.
🧹 Nitpick comments (5)
src/utils/units.ts (1)
264-266: Minor: Typo in comment.Line 266 has "perent" instead of "percent".
Proposed fix
- { style: 'percent' }, // Don't allow perent style to be overridden. + { style: 'percent' }, // Don't allow percent style to be overridden.DEPENDENCIES.md (1)
18-20: Add language specifier to fenced code block.The error message code block is missing a language identifier, which triggers markdownlint MD040.
📝 Suggested fix
-``` +```text TypeError: The 'compilation' argument must be an instance of Compilation</details> </blockquote></details> <details> <summary>src/components/shared/OverviewDetailItem.tsx (1)</summary><blockquote> `33-33`: **Consider using template literals for className concatenation.** Minor readability improvement for the className logic. <details> <summary>♻️ Optional refactor</summary> ```diff - <dd className={valueClassName ? `ng-overview-detail-item__value ${valueClassName}` : 'ng-overview-detail-item__value'}>{value}</dd> + <dd className={`ng-overview-detail-item__value${valueClassName ? ` ${valueClassName}` : ''}`}>{value}</dd>Or use a utility like
clsxif already available in the project:<dd className={clsx('ng-overview-detail-item__value', valueClassName)}>{value}</dd>i18next-parser.config.js (1)
1-2: Minor: Consider using consistent eslint-disable syntax.The two eslint-disable comments can be consolidated for cleaner code.
Suggested consolidation
-// eslint-disable-next-line `@typescript-eslint/no-var-requires`, no-undef -const { CustomJSONLexer } = require('./i18n-scripts/lexers'); - -// eslint-disable-next-line no-undef +/* eslint-disable `@typescript-eslint/no-var-requires`, no-undef */ +const { CustomJSONLexer } = require('./i18n-scripts/lexers'); +This uses a single block-level disable at the top since
no-undefapplies to the entire file (formodule.exports).i18n-scripts/common.js (1)
14-27: Consider simplifying the async function pattern.The IIFE pattern
return (async () => { ... })()is functional but unnecessarily complex. Anasyncfunction directly would be cleaner and more readable.Suggested simplification
- parseFolder(directory, argFunction, packageDir) { - return (async () => { - try { - const files = await fs.promises.readdir(directory); - for (const file of files) { - const filePath = path.join(directory, file); - await argFunction(filePath, packageDir); - } - } catch (e) { - console.error(`Failed to parseFolder ${directory}:`, e); - throw e; - } - })(); - }, + async parseFolder(directory, argFunction, packageDir) { + try { + const files = await fs.promises.readdir(directory); + for (const file of files) { + const filePath = path.join(directory, file); + await argFunction(filePath, packageDir); + } + } catch (e) { + console.error(`Failed to parseFolder ${directory}:`, e); + throw e; + } + },
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (34)
.dockerignore.eslintrc.eslintrc.yml.gitignore.prettierrc.yml.stylelintrc.yamlDEPENDENCIES.mdDockerfileREADME.mddeployment/console-plugin-nvidia-gpu/Chart.yamli18n-scripts/build-i18n.shi18n-scripts/common.jsi18n-scripts/lexers.jsi18n-scripts/set-english-defaults.jsi18next-parser.config.jslocales/en/plugin__console-plugin-nvidia-gpu.jsonpackage.jsonsrc/components/ClusterOverview/GPUProviders.tsxsrc/components/ClusterOverview/GPUStatus.tsxsrc/components/GPUDashboard/Cards/InfoCard.tsxsrc/components/GPUDashboard/Cards/MetricsCard.csssrc/components/GPUDashboard/Cards/MetricsCard.tsxsrc/components/GPUDashboard/Cards/WorkloadsCard.csssrc/components/GPUDashboard/Cards/WorkloadsCard.tsxsrc/components/GPUDashboard/GPUDashboard.tsxsrc/components/GPUDashboard/GPUSelector.tsxsrc/components/NotAvailable.csssrc/components/NotAvailable.tsxsrc/components/PodStatus/PodStatus.tsxsrc/components/shared/OverviewDetailItem.csssrc/components/shared/OverviewDetailItem.tsxsrc/utils/units.tstsconfig.jsonwebpack.config.ts
💤 Files with no reviewable changes (2)
- .prettierrc.yml
- .eslintrc
🚧 Files skipped from review as they are similar to previous changes (20)
- src/components/NotAvailable.css
- src/components/GPUDashboard/Cards/MetricsCard.css
- src/components/NotAvailable.tsx
- src/components/ClusterOverview/GPUStatus.tsx
- src/components/GPUDashboard/Cards/MetricsCard.tsx
- src/components/GPUDashboard/GPUSelector.tsx
- .gitignore
- i18n-scripts/set-english-defaults.js
- deployment/console-plugin-nvidia-gpu/Chart.yaml
- .dockerignore
- .eslintrc.yml
- tsconfig.json
- i18n-scripts/lexers.js
- src/components/shared/OverviewDetailItem.css
- Dockerfile
- src/components/GPUDashboard/Cards/InfoCard.tsx
- .stylelintrc.yaml
- i18n-scripts/build-i18n.sh
- src/components/GPUDashboard/Cards/WorkloadsCard.tsx
- README.md
🧰 Additional context used
🧬 Code graph analysis (2)
src/components/GPUDashboard/GPUDashboard.tsx (1)
src/components/GPUDashboard/Cards/GPUDashboardGraphs.tsx (1)
GPUDashboardGraphs(48-146)
i18n-scripts/common.js (2)
i18n-scripts/set-english-defaults.js (3)
fs(1-1)path(2-2)file(9-9)i18next-parser.config.js (1)
require(2-2)
🪛 markdownlint-cli2 (0.18.1)
DEPENDENCIES.md
30-30: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (19)
src/components/GPUDashboard/Cards/WorkloadsCard.css (1)
1-9: LGTM!The missing closing brace fix and the new
.ng-break-wordutility class look good. The class correctly replaces the removedco-break-worddependency, and the naming follows the project'sng-prefix convention.Note:
word-break: break-wordis technically deprecated, but it serves as a reasonable fallback for older browsers alongside the modernoverflow-wrap: anywhere. This is fine to keep as-is.src/utils/units.ts (4)
179-187: LGTM!The type signature update to accept
string | numberaligns well with the SDK upgrade and provides flexibility for callers passing string values from API responses while maintaining type safety through explicitNumber()coercion in the implementations.
189-208: LGTM!Consistent
Number(v)coercion across all wrapper functions. The downstreamhumanize()function already handles non-finite values (line 147-149), so invalid string inputs like"abc"will safely normalize to 0.
209-221: LGTM!The coercion and normalization logic is correct. Input handling is consistent with other functions in this file.
223-249: LGTM!The coercion pattern is consistent with the rest of the file. The semantic distinction between
humanizeRatio(input 0.5 → "50%") andhumanizePercentage(input 50 → "50%") is correctly preserved.src/components/GPUDashboard/GPUDashboard.tsx (1)
26-55: LGTM! Clean PatternFly v6 layout structure.The simplified layout using a single
PageSectionwithStack,Split, andGridcomponents is well-organized and follows PatternFly v6 patterns. The responsive grid layout for MIGCard and InfoCard is appropriate.DEPENDENCIES.md (1)
1-40: Valuable dependency documentation.This documentation will help future maintainers understand the version constraints and their rationale. The clear structure with "Reason", "Impact", and "Resolution" sections is excellent.
src/components/PodStatus/PodStatus.tsx (1)
116-149: Clean PatternFly v6 migration for footer content.The refactor from
Text/TextVariantstoContentcomponents withcomponent="p"is correct for PatternFly v6. The fragment wrapper and structure maintain proper semantics.webpack.config.ts (3)
61-73: DevServer configuration is appropriate for plugin development.The
allowedHosts: 'all'and broad CORS headers are acceptable for development since this configuration only applies towebpack-dev-serverand is necessary for local testing with the OpenShift console. These settings do not affect production builds.
43-44: The SCSS loader configuration is properly supported by existing devDependencies.All required packages (
sass-loader@^13.3.0,sass@^1.77.0,css-loader@^6.7.1, andstyle-loader@^3.3.1) are already present in devDependencies. The loader chain ordering is correct.
76-78: Locales directory path is correct.The
localesdirectory exists at the project root with proper structure (containing theensubdirectory), andpath.resolve(__dirname, 'locales')correctly resolves to it. The CopyWebpackPlugin configuration is valid.src/components/shared/OverviewDetailItem.tsx (1)
14-36: Clean implementation of the shared detail item component.The component correctly handles loading, error, and content states. The use of semantic
dl/dt/ddelements is appropriate for definition list semantics. The error styling issue from the previous review has been addressed with the dedicated CSS class.src/components/ClusterOverview/GPUProviders.tsx (1)
2-2: LGTM!The import path update from
@openshift-console/plugin-sharedto the local shared component aligns with the SDK upgrade. This properly decouples the component from the external SDK dependency.package.json (3)
5-8: LGTM!Good additions - explicit Apache-2.0 license and Node.js 20+ engine requirement align with the PR objectives for OpenShift 4.19+ compatibility.
9-19: LGTM!The script updates are well-structured:
- The
webpackhelper script properly usests-node/registerfor TypeScript config support- The
type-checkscript enables TypeScript strict mode validation- The
i18nworkflow integrates the new shell-based extraction pipeline
82-84: LGTM!The
i18nextpeer dependency is correctly specified. This ensures the host application provides the i18next instance for proper module federation sharing.locales/en/plugin__console-plugin-nvidia-gpu.json (1)
1-85: LGTM!The locale file correctly implements i18next pluralization. The
_one/_othersuffixes are properly used in keys while values contain the actual display text (e.g.,"{{count}} GPU pod_one": "{{count}} GPU pod"). The GPU-centric terminology migration is comprehensive and consistent.i18next-parser.config.js (1)
15-30: LGTM!The lexers configuration correctly maps file extensions to their appropriate parsers. The
CustomJSONLexeris properly passed as a class reference (not a string) which is the correct pattern for custom lexers in i18next-parser.i18n-scripts/common.js (1)
4-36: LGTM overall - utility functions are well-structured.The error handling is consistent across all three utilities, and the module exports provide a clean API for the i18n scripts. The synchronous/asynchronous choices are appropriate for their use cases.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
/lgtm |
openshift-merge-bot
bot
merged commit 63fa766
into
rh-ecosystem-edge:main
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Style
Chores
✏️ Tip: You can customize this high-level summary in your review settings.